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

Make SendNode#macro? aware of struct constructor #141

Merged
merged 1 commit into from Oct 23, 2020
Merged

Make SendNode#macro? aware of struct constructor #141

merged 1 commit into from Oct 23, 2020

Conversation

koic
Copy link
Member

@koic koic commented Oct 22, 2020 •
edited

This PR makes SendNode#macro? aware of struct constructor.

Like class constructor, struct constructor will be recognized as a macro and will be awakened in the following private modifier:

Foo = Struct.new(:foo) do
  private
    def private_foo
      foo
    end
end

With this change, this PR aims to resolve the following issue.
rubocop/rubocop#8919

@koic koic changed the title Support struct constructor for SendNode#macro? Make SendNode#macro? aware of struct constructor Oct 22, 2020
@koic koic force-pushed the support_struct_constructor_send_node_macro branch from 4fc1057 to 008a5c9 Compare October 22, 2020 10:14
@marcandre
Copy link
Contributor

Very good. I consider that a bugfix.

Only thing I'm wondering... Should we not include Struct.new into #class_constructor?

I looked at the code in the main gem, and there is one class_constructor? || struct_constructor? and the only other use of class_constructor? (Lint/DuplicateMethods) should also include struct constructor.

I think it would be best to deprecate struct_constructor? and include that check in class_constructor?, or else create a module_constructor? that would include everything and deprecate both class_constructor? and struct_constructor?...

@koic
Make SendNode#macro? aware of struct constructor
ca2da26 
This PR makes `SendNode#macro?` and `RuboCop::AST::Node#class_constructor?`
aware of struct constructor and `RuboCop::AST::Node#struct_constructor?`
is deprecated.

Like class constructor, struct constructor will be recognized as a
macro and will be awakened in the following `private` modifier:

```ruby
Foo = Struct.new(:foo) do
  private
    def private_foo
      foo
    end
end
```

With this change, this PR aims to resolve the following issue.
rubocop/rubocop#8919
@koic koic force-pushed the support_struct_constructor_send_node_macro branch from 008a5c9 to ca2da26 Compare October 23, 2020 04:25
@koic
Copy link
Member Author

koic commented Oct 23, 2020

I think it would be best to deprecate struct_constructor? and include that check in class_constructor?

Thank you for your review. I updated this PR with the suggestion.

bbatsov
bbatsov approved these changes Oct 23, 2020
View reviewed changes
Copy link
Contributor

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

I like the change.

@marcandre marcandre merged commit 827c1ed into rubocop:master Oct 23, 2020
@marcandre
Copy link
Contributor

Perfect, thanks!

@koic koic deleted the support_struct_constructor_send_node_macro branch October 23, 2020 06:50
@marcandre
Copy link
Contributor

Released in 1.0.1

@koic
Copy link
Member Author

koic commented Oct 23, 2020

@marcandre Wow! Thank you for the quick release ❤️

koic added a commit to koic/rubocop that referenced this pull request Oct 23, 2020
@koic
[Fix rubocop#8919] Require RuboCop AST to 1.0.1 or higher
ecf4316 
Fixes rubocop#8919 and follow rubocop/rubocop-ast#141.

This PR requires rubocop-ast 1.0.1 to awaken of struct constructor.
And it will also add a reproduction test for rubocop#8919
@koic koic mentioned this pull request Oct 23, 2020
Merged
8 tasks
@marcandre
Copy link
Contributor

Of course. FWIW, I intend to release bug fixes within a day of merging. It's so easy to do a release and there's not that many PRs for rubocop-ast...

@koic koic mentioned this pull request Oct 23, 2020
Closed
koic added a commit to rubocop/rubocop that referenced this pull request Feb 8, 2023
@koic
Remove deprecated redundant code
9ee83ab 
Follow up rubocop/rubocop-ast#141.

`RuboCop::AST::Node#struct_constructor?` has been deprecated and
merged into `RuboCop::AST::Node#class_constructor`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbatsov bbatsov bbatsov approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@koic @marcandre @bbatsov