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

Improve parent_module_name #177

Merged
merged 3 commits into from
May 26, 2021
Merged

Improve parent_module_name #177

merged 3 commits into from
May 26, 2021

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Apr 5, 2021

This builds on #176

@sanfrecce-osaka:

  • first commit is yours
  • second one shows a way to simplify the tests.
  • last commit adds a test that would fail with your patch, and fixes that.

This still isn't quite right as the result with duplicate_method cop isn't quite right, and it gets confused between Foo.method and Foo::#<Class:Foo>#method (correct but not as nice).

I'm not sure if it's best to change the cop, or add an Node method for qualified_name or something.

sanfrecce-osaka and others added 3 commits April 5, 2021 13:35
With this PR, when Rubocop::AST::Node#parent_module_name_part is executed, it will no longer do non-local exits when a block is evaluated, and evaluating singleton_class or block in Rubocop::AST::Node#parent_module_name will behave as expected.
With this change, this PR aims to resolve the following issue.

rubocop/rubocop#5022
@bbatsov
Copy link
Contributor

bbatsov commented May 26, 2021

I'm fine with both proposals, although I'm leaning slightly towards the second option.

@marcandre marcandre merged commit e303401 into rubocop:master May 26, 2021
@marcandre marcandre deleted the parent_mod branch May 26, 2021 14:14
@marcandre
Copy link
Contributor Author

Merging as is, further improvements welcome

@marcandre
Copy link
Contributor Author

I'm not sure how come the test suite was green. This deteriorates even some simple cases. Reverted.

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.

3 participants