Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add forwarded_kwrestarg node to AST::Builder #259

Merged
merged 1 commit into from
Mar 24, 2023
Merged

Add forwarded_kwrestarg node to AST::Builder #259

merged 1 commit into from
Mar 24, 2023

Conversation

koic
Copy link
Member

@koic koic commented Mar 24, 2023

This PR adds forwarded_kwrestarg node to AST::Builder.

This feature is required to resolve rubocop/rubocop#11730.

I thought about defining a class like ForwardedKwrestargNode for forwarded_kwrestarg, but the node is actually an anonymous KeywordSplatNode so it's used.

@koic koic force-pushed the add_forwarded_kwrestarg_to_ast_builder branch from 072934b to 6dc37dd Compare March 24, 2023 03:44
koic
koic commented Mar 24, 2023
View reviewed changes
lib/rubocop/ast/builder.rb
ensure: EnsureNode,
for: ForNode,
forward_args: ForwardArgsNode,
forwarded_kwrestarg: KeywordSplatNode,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is the only one added in this map.

@koic koic mentioned this pull request Mar 24, 2023
Merged
8 tasks
@koic koic force-pushed the add_forwarded_kwrestarg_to_ast_builder branch from 6dc37dd to 6f0320a Compare March 24, 2023 05:14
@marcandre
Copy link
Contributor

My only hesitation is: do we want a different class, or could we not use Kwsplat?

@marcandre marcandre mentioned this pull request Mar 24, 2023
Closed
@marcandre
Copy link
Contributor

My only hesitation is: do we want a different class, or could we not use Kwsplat?

I was clearly tired, sorry, you are using it 👍 .

I guess my hesitation was more should kwsplat_type? return true...

@koic koic force-pushed the add_forwarded_kwrestarg_to_ast_builder branch from 6f0320a to 7f31e0f Compare March 24, 2023 15:48
@koic
Copy link
Member Author

koic commented Mar 24, 2023

Ah, yeah, I thought it would be better to be able to distinguish between kwsplat node type and forwarded_kwrestarg node type, but it seems pragmatic and more better to have compatibility with kwsplat_type? as you suggested. I've updated this PR.

@marcandre
Copy link
Contributor

kwsplat_type

Oh, good. I'm not sure, but yeah, seems like usually we won't really care about the distinction. Thanks

@koic
Add forwarded_kwrestarg node to AST::Builder
325060a 
This PR adds `forwarded_kwrestarg` node to `AST::Builder`.

This feature is required to resolve rubocop/rubocop#11730.

I thought about defining a class like `ForwardedKwrestargNode` for `forwarded_kwrestarg`,
but the node is actually an anonymous `KeywordSplatNode` so it's used.
@koic koic force-pushed the add_forwarded_kwrestarg_to_ast_builder branch from 7f31e0f to 325060a Compare March 24, 2023 16:54
@marcandre marcandre merged commit 27d62fa into rubocop:master Mar 24, 2023
@marcandre
Copy link
Contributor

Thank you so much! Released as 1.28.0 🚀

@koic koic deleted the add_forwarded_kwrestarg_to_ast_builder branch March 25, 2023 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcandre marcandre marcandre left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

It appears Layout/HashAlignment don't like anonymous arguments
2 participants
@koic @marcandre