Skip to content

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Aug 11, 2025

Fixes
image

@AndreasArvidsson AndreasArvidsson requested a review from a team as a code owner August 11, 2025 02:43
@AndreasArvidsson AndreasArvidsson merged commit 9cb2a3b into main Aug 11, 2025
15 checks passed
@AndreasArvidsson AndreasArvidsson deleted the rArgument branch August 11, 2025 02:59
@AndreasArvidsson AndreasArvidsson changed the title Fix bug wit R arguments and incidental multiple matches Fix bug with R arguments and incidental multiple matches Aug 11, 2025
@pokey
Copy link
Member

pokey commented Aug 11, 2025

...test? 😄

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Aug 11, 2025

...test? 😄

I was a bit hesitant on this one. We already have argument tests for this language. The problem only occurred when you had three or more arguments. I could of course add that test, but right now we have test parity between the languages due to the scope facets. If I add an argument test with three arguments here should I do it for all languages? Or maybe I should just add it here because the Tree sitter looks a bit different from other languages? Should that be a special facet for this and may be other languages? To be honest I'm not sure so I went with the old "When in doubt: do nothing" :D

@pokey
Copy link
Member

pokey commented Aug 11, 2025

Hmm good question. Simplest is just to add another test for R with 3 args using the actualArg facet. I think there's always going to be some variation between languages. Alternatively, you could add a new facet called actualArg.multiple or something like that, and then add it for all languages. But sorta goes combinatoric because you'd prob want that for parameters, etc. But I think one test that introduces slight variation between languages is better than nothing. I think "When in doubt, write a test" is probably a better mantra? 😅

@AndreasArvidsson
Copy link
Member Author

Sounds reasonable :)

#3066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants