-
Notifications
You must be signed in to change notification settings - Fork 426
Groovy support for multiple assignment/tuple unpacking #5293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
sudouser777
commented
Apr 14, 2025
- Updated the groovy parser to support tuple unpacking.
- closes Groovy parser fails with parentheses on the left-hand side of the assignment #5283 .
I have tried to hanlde the spaces situation, but it's tricky spaces are not being handled properly
Also static type also not working
@greg-at-moderne I'm not sure if my approach is correct—open to any suggestions. |
rewrite-groovy/src/test/java/org/openrewrite/groovy/tree/DestructuringTest.java
Outdated
Show resolved
Hide resolved
namedVariables.set(first, namedVariables.get(first).withPrefix(prefixBeforeOpenParentheses).withMarkers(Markers.build(singletonList(new OpenParentheses(randomId()))))); | ||
namedVariables.set(last, namedVariables.get(last).withMarkers(Markers.build(singletonList(new CloseParentheses(randomId()))))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is usually a JContainer
that holds the named variables, then wrapped inside of a J.Parentheses
instead. This then means that the markers wouldn't be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's not including parenthesis, it's just returning JRightPadded<J.Identifier>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more in general. The way to add parentheses around an element is with J.Parentheses
and when that element is a list it is typically wrapped in a JContainer
.
I'm not sure what elements you currently have in scope from the Groovy AST, but I'd expect that something similar would be the case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's using the same J.VariableDeclarations
for normal variable declarations. This will require changes to the J.VariableDeclarations
class. I'm not entirely sure about this approach, though.
rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyParserVisitor.java
Outdated
Show resolved
Hide resolved
rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyParserVisitor.java
Outdated
Show resolved
Hide resolved
rewrite-groovy/src/main/java/org/openrewrite/groovy/GroovyParserVisitor.java
Outdated
Show resolved
Hide resolved
Thanks a lot for the continued improvements to the Groovy parser @sudouser777 ! Great to see steady progress being made. Any reason you'd held off on a merge here @greg-at-moderne ? |
namedVariables = ListUtils.mapFirst(namedVariables, first -> first.withPrefix(prefixBeforeOpenParentheses).withMarkers(Markers.build(singletonList(new OpenParentheses(randomId()))))); | ||
namedVariables = ListUtils.mapLast(namedVariables, last -> last.withMarkers(Markers.build(singletonList(new CloseParentheses(randomId()))))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than adding new markers, is there any way we can make this work with regular J.Parentheses
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's using J.VariableDeclarations
, which is also used for normal variable declarations. If we add J.Parentheses
, I'm not sure how many changes it would require in the existing code, and I'm also not sure whether it's possible to make the parentheses optional.
I'm open to suggestions—let me know if there's a better way we can handle this.
No. I am good. I think this is good to be merged now. |