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

Mark Naming/RescuedExceptionsVariableName as unsafe #6994

Conversation

vfonic
Copy link
Contributor

@vfonic vfonic commented Apr 29, 2019

I just ran rubocop --safe-auto-correct and it made this autocorrect change:

-   rescue ActiveResource::Redirection => redirection
+   rescue ActiveResource::Redirection => e
      redirect_to redirection.response['Location']
    end

This is obviously a breaking change. I believe the easiest fix here is to mark this cop as unsafe, as, as far as I know, rubocop doesn't offer the extensive functionality for renaming a variable (redirection) across all of its usages.

I'm open for suggestions for better fix.

EDIT: I dug down and realized this has just recently been added by @anthony-robin (Thanks!) in this PR #6905. @anthony-robin, what do you think of marking this cop unsafe?


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • 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.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@vfonic vfonic force-pushed the naming-rescued-exceptions-variable-name-unsafe branch from f1f45dd to c70e9cd Compare April 29, 2019 18:11
VersionAdded: '0.67'
VersionChanged: '0.68'
VersionChanged: '0.69'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about this...

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 29, 2019

The cop itself is safe (meaning it doesn't generate false positives), only it's auto-correct should be marked as unsafe.

@Darhazer
Copy link
Member

I've opened #6998 fixing the auto-correct to rename the variable inside the body as well

@vfonic
Copy link
Contributor Author

vfonic commented Apr 29, 2019

The cop itself is safe (meaning it doesn't generate false positives), only it's auto-correct should be marked as unsafe.

@bbatsov I didn't know that's what "safe" means. Thanks!

@Darhazer awesome! :) Didn't know this is possible. I shouldn't have underestimated rubocop. :)
I'm gonna go ahead and close this PR, your PR is the one that should be merged.

@vfonic vfonic closed this Apr 29, 2019
@vfonic vfonic deleted the naming-rescued-exceptions-variable-name-unsafe branch April 29, 2019 21:34
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

3 participants