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

Don't add Style/Alias offense for use of alias in eval_instance blocks #3004

Closed
wants to merge 1 commit into from

Conversation

magni-
Copy link
Contributor

@magni- magni- commented Apr 5, 2016

This fixes an issue with the Style/Alias cop. instance_eval blocks are found to be dynamic scope, so Rubocop currently adds an offense when using alias instead of alias_method, but object instances do not respond to alias_method.


Before submitting a PR make sure the following are checked:

  • Wrote good commit messages.
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it)
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@magni- magni- force-pushed the instance_eval_alias branch 2 times, most recently from 59fd4a9 to c053edf Compare April 11, 2016 06:51
@@ -27,8 +27,11 @@ def on_send(node)
def on_alias(node)
# alias_method can't be used with global variables
return if node.children.any?(&:gvar_type?)
# alias_method can't be used in instance_eval blocks
scope_type = scope_type(node)
return if @must_use_alias
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once set, the value of @must_use_alias remains and affects the inspection of the next alias found in the same file. I think it's better if you add a third scope type called :instance_eval.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good catch, will fix 👍

@magni- magni- force-pushed the instance_eval_alias branch from c053edf to 0db9ef9 Compare April 16, 2016 03:44
@jonas054
Copy link
Collaborator

👍

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 23, 2016

@magni- Squash the commit together and rebase on top of the current master.

@magni- magni- force-pushed the instance_eval_alias branch from 0db9ef9 to 5720ed5 Compare April 24, 2016 14:46
@magni-
Copy link
Contributor Author

magni- commented Apr 25, 2016

@bbatsov done!

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 26, 2016

Had to rebase this once more myself. The commit is merged now.

@bbatsov bbatsov closed this Apr 26, 2016
@magni-
Copy link
Contributor Author

magni- commented Apr 27, 2016

Thanks!

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