Migrate java scopes #2038
Migrate java scopes #2038
Conversation
| consequence: (block) @branch.end @condition.domain.end | ||
| ) @dummy | ||
| (#not-parent-type? @dummy "if_statement") | ||
| (#child-range! @condition 0 -1 true true) |
There was a problem hiding this comment.
This includes any whitespace surrounding the condition inside the parens. Probably fine. Def makes sense for removal range. Prob good enough for content range
There was a problem hiding this comment.
I think so. Otherwise I definitely think that we need a trim predicate here.
| consequence: (_) @branch | ||
| ) @condition.domain | ||
| (ternary_expression | ||
| alternative: (_) @branch |
There was a problem hiding this comment.
Do we have a way to define a scope multiple places in the same pattern?
There was a problem hiding this comment.
unfortunately not. The problem is that then things like .leading, .start, etc become ambiguous. I guess we could allow it if we don't see any of those things in the pattern? 🤔 cc/ @josharian
There was a problem hiding this comment.
I have now forgotten all of the details but last time I tried playing games like that I ended up in a tangled mess of code. I will generally choose clear over clever/concise, personally.
There was a problem hiding this comment.
Couldn't we number the scopes?
(ternary_expression
consequence: (_) @branch1
alternative: (_) @branch2
)
If we can't use actual numbers we could use letters or roman numerals? :D
There was a problem hiding this comment.
That's an interesting idea. Or @branch.1
There was a problem hiding this comment.
Yeah probably even better
auscompgeek
added
the
scope-migration
There was a problem hiding this comment.
Ok I didn't look super closely, as it's been awhile since my java days, we have lots of tests, and you're using it day-to-day so will notice oddnesses. I left a few comments. In addition, the following looked weird to me for "value"
domain should include ; above; might be same issue I point out in inline comment
these should be values
| consequence: (_) @branch | ||
| ) @condition.domain | ||
| (ternary_expression | ||
| alternative: (_) @branch |
There was a problem hiding this comment.
unfortunately not. The problem is that then things like .leading, .start, etc become ambiguous. I guess we could allow it if we don't see any of those things in the pattern? 🤔 cc/ @josharian
|
@pokey Updated. |
queries/java.scm
Outdated
| (local_variable_declaration | ||
| (variable_declarator | ||
| name: (_) @name @value.removal.start.endOf | ||
| value: (_)? @value @value.removal.end | ||
| ) | ||
| ) @_.domain | ||
| (field_declaration | ||
| (variable_declarator | ||
| name: (_) @name @value.removal.start.endOf | ||
| value: (_)? @value @value.removal.end | ||
| ) | ||
| ) @_.domain | ||
|
|
||
| ;;!! value = 1; | ||
| ;;! ^ | ||
| ;;! xxxx | ||
| ;;! ---------- | ||
| (_ | ||
| (assignment_expression | ||
| left: (_) @value.removal.start.endOf | ||
| right: (_) @value @value.removal.end | ||
| ) @_.domain.start | ||
| ";"? @_.domain.end | ||
| ) |
There was a problem hiding this comment.
I'd argue for using leading / trailing instead of removal for all of these
There was a problem hiding this comment.
If you prefer I could do that. Personally I don't think I have every used leading and trailing for anything else than whitespaces, but I'm not against it.
There was a problem hiding this comment.
Ha yeah was kind of thinking the same thing in the back of my mind 😅. Better to be consistent tho I guess 🤷♂️
#Fixes #1926
Checklist