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.

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

Only traverse send nodes in def_modifier? #142

Merged
merged 1 commit into from Oct 22, 2020

Conversation

eugeneius
Copy link
Contributor

@eugeneius eugeneius commented Oct 22, 2020

A send node is a "def modifier" if it's part of a chain of method calls preceding a method definition. However the previous implementation was too permissive and matched any method definition in the send node's AST.

In the following example, include was considered to be a def modifier:

include Module.new {
  private def foo
  end
}

This is the root cause of the problem fixed in rubocop/rubocop#8653.

A send node is a "def modifier" if it's part of a chain of method calls
preceding a method definition. However the previous implementation was
too permissive and matched any method definition in the send node's AST.

In the following example, `include` was considered to be a def modifier:

    include Module.new {
      def foo
      end
    }
@marcandre
Copy link
Contributor

Super!
Thanks for using the new changelog method. I'll make it clearer, but you didn't even have to enter your own nick's entry at the end of the Changelog.

@marcandre marcandre merged commit 12734b6 into rubocop:master Oct 22, 2020
@eugeneius
Copy link
Contributor Author

Oh, nice. I had originally added the changelog entry directly, but I moved it after reading CONTRIBUTING.md and left the link definition in place. I think the instructions are pretty clear, I'm just used to the old way.

@marcandre
Copy link
Contributor

👍 Perfect then.
I intend to make a release tomorrow btw.

@marcandre
Copy link
Contributor

Actually, I released 1.0.1 now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants