-
Notifications
You must be signed in to change notification settings - Fork 4
Require prism >= 0.19.0 #4
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
Conversation
st0012
left a comment
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.
If I understand it correctly, after this change we'll be supporting both Prism 0.18 and 0.19? If that's the case, maybe we should just move towards 0.19 as people should always upgrade Prism, especially before Ruby 3.3?
In the meantime, we can point Prism to GH source in the Gemfile.
|
I was thinking of:
Mainly to make CI green. (but it makes code complicated...) Using github source in Gemfile seems good, I'll try it. |
74fa4ab to
d518e98
Compare
| end | ||
| if node.rest&.name | ||
| # node.rest is Prism::RestParameterNode | ||
| if node.rest.is_a?(Prism::RestParameterNode) && node.rest.name |
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.
There's a if node.rest condition a few lines above, which seems to already expect it to be Prism::RestParameterNode? Perhaps we can merge that and this together?
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.
I removed the confusing comment in the if node.rest condition and add a comment about Prism::ImplicitRestNode that does not have name.
The if node.rest above part is separating given args to reqs, posts, rest
method do |a,b=1,*c| and given args [Integer, String, Symbol] to reqs = [Integer], opts = [String], posts = [], rest = [Symbol].
This part is assigning each of them.
node.requireds.zip{assign}
node.optionals.zip{assign}
node.posts.zip{assign}
if node.rest; assign; endI want to separate them, not to merge it together.
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.
I see, thanks for the clear explanation 👍
Remove StringNode workaround
Simplify CallNode attribute assign check (
CallNode#attribute_write?is added)Fix for added nodes
NumberedParametersNodeCallTargetNodeIndexTargetNodeImplicitRestNode