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

Ignore redirect method for Style/FormatStringToken by default #902

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

javierjulio
Copy link
Contributor

@javierjulio javierjulio commented Dec 30, 2022

This adds the redirect method as a default for the Style/FormatStringToken rule due to the change in rubocop/rubocop#9369 where it was mentioned that it would be added as a default but since it wasn't, I'm contributing the change. I ran into this false positive with a rails route using both rubocop and rubocop-rails. Seems like a good default to have, if still desired.


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.
  • If this is a new cop, consider making a corresponding update to the Rails Style Guide.

@@ -1143,3 +1143,7 @@ Style/SymbolProc:
- define_method
- mail
- respond_to

Style/FormatStringToken:
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I made that change. I noticed shortly after creating this PR though that the IgnoredMethods key is deprecated and that AllowedMethods is used instead. I updated the config to match what I saw used elsewhere. Let me know if that's ok or if there are more changes needed.

Copy link

@pboling pboling Jan 25, 2023

Choose a reason for hiding this comment

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

@koic @javierjulio Why was IgnoredMethods replaced with AllowedMethods? They have opposite meanings, and the documentation is also opposite.

The documentation for IgnoredMethods was:

This module encapsulates the ability to ignore certain methods when parsing

The documentation for AllowedMethods is the opposite:

This module encapsulates the ability to allow certain methods when parsing.

How are these interchangeable?

The implementation of AllowedMethods does the opposite of what the documentation says it does, as it does in fact ignore the method (not allow it). This means that the documentation is wrong, and even if it was fixed, the implied meaning is different form what it does, and that there is no way to actually allow a method, only to disallow (i.e. ignore).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went off of other rules listed in the same file that have the same keys and "deprecated" notice. Since they listed both keys with the same list and since there wasn't explicit review follow up, I figured that was the right solution. If it's not though, it should be changed.

Copy link

Choose a reason for hiding this comment

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

Current behavior is correct, but the documentation confused me.

rubocop/rubocop#10829 (comment)

@koic
Copy link
Member

koic commented Jan 4, 2023

Can you add a changelog entry and squash your commits into one?

@koic
Copy link
Member

koic commented Jan 6, 2023

@javierjulio ping :-)

@javierjulio
Copy link
Contributor Author

@koic thank you for the ping! 🙏🏻 I'm sorry, I didn't get an email notification about your review so I missed it. Let me update this for you.

@javierjulio javierjulio changed the title Ignore redirect method for Style/FormatStringToken Ignore redirect method for Style/FormatStringToken by default Jan 6, 2023
@javierjulio javierjulio changed the title Ignore redirect method for Style/FormatStringToken by default Ignore redirect method for Style/FormatStringToken by default Jan 6, 2023
This adds the `redirect` method as a default for the Style/FormatStringToken rule due to the change in rubocop/rubocop#9369 where it was mentioned that it would be added as a default.
@javierjulio
Copy link
Contributor Author

@koic all set. I added the changelog entry as well.

@koic koic merged commit c33770b into rubocop:master Jan 6, 2023
@koic
Copy link
Member

koic commented Jan 6, 2023

@javierjulio Great! Thank you!

@javierjulio
Copy link
Contributor Author

@koic no, thank you! ❤️

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