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

[Fix #10427] Style/For: Mark auto-correction as unsafe #10429

Merged
merged 1 commit into from Feb 21, 2022
Merged

[Fix #10427] Style/For: Mark auto-correction as unsafe #10429

merged 1 commit into from Feb 21, 2022

Conversation

issyl0
Copy link
Contributor

@issyl0 issyl0 commented Feb 19, 2022

The only thing I didn't do in the PR checklist is add tests, because a recent, different PR to mark a cop as unsafe (10408) didn't have any. Hope that's OK!


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@issyl0
Copy link
Contributor Author

issyl0 commented Feb 19, 2022

Oops, my commit message started [Fixes] instead of [Fix] in the present tense. I force-pushed to amend according to your convention. Looks like someone will have to re-approve the "first time contributor workflow" Actions CI run again, my apologies.

Also, do I have to do anything here to get the CircleCI tests to run?

@issyl0 issyl0 changed the title [Fixes #10427] Style/For: Mark auto-correction as unsafe [Fix #10427] Style/For: Mark auto-correction as unsafe Feb 19, 2022
@jonas054
Copy link
Collaborator

I don't understand why the CircleCi runs have not started.

@jonas054 jonas054 self-requested a review February 20, 2022 07:44
@@ -0,0 +1 @@
* [#10427](https://github.com/rubocop/rubocop/issues/10427): Mark `Style/For` as unsafe auto-correction. ([@issyl0][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, as a first time contributor, you need to add your name at the bottom of CHANGELOG.md. See the last bullet point on https://github.com/rubocop/rubocop/blob/master/CONTRIBUTING.md.

Copy link
Member

Choose a reason for hiding this comment

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

There is no need to manually update the CHANGELOG.md file as first time contributor name will be inserted automatically :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's great! So much automation going on that I haven't kept up with.

config/default.yml Outdated Show resolved Hide resolved
lib/rubocop/cop/style/for.rb Outdated Show resolved Hide resolved
@jonas054
Copy link
Collaborator

I've set "approve", since my comment turned out to be unnecessary. @issyl0 Let's hope all the build steps kick off if you push the changes @koic wanted.

- The Ruby Style Guide (https://rubystyle.guide/#no-for-loops) states
  that "for doesn’t introduce a new scope (unlike each) and variables
  defined in its block will be visible outside it". This means that
  auto-correcting can break existing, working code because of the change
  in variable scope.
@issyl0
Copy link
Contributor Author

issyl0 commented Feb 20, 2022

Thanks for the review, @jonas054 and @koic. I made the changes you requested.

@koic
Copy link
Member

koic commented Feb 21, 2022

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.

Style/For should be marked as unsafe
3 participants