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 false-positives that non Rails formats are offended on Rails/ToSWithArgument #869

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

r7kamura
Copy link
Contributor

We had Rails/ToSWithArgument Cop to replace to_s with to_fs where Rails' extended functionality is used, but there are frequent false-positives where to_s is used with non Rails extended formats like value.to_s(2) (Integer#to_s).

To prevent this, it would be better to check its 1st argument so that only cases that appear to use Rails-extended functionality are offended.


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.

@r7kamura
Copy link
Contributor Author

Let me provide a little more background.

To prepare to set config.active_support.disable_to_s_conversion = true (which is also set by load_defaults 7.0), I've tested this Cop against the source code of all the Gems that my Rails app depends on, and I noticed that there were so many false-positives. I ended up inspecting 20000+ Ruby files and found that 2 files still used the Rails extended to_s 😨, and there are 200+ false-positives about Integer#to_s.

This is why I decided to add this fix.

@@ -1,5 +1,7 @@
# frozen_string_literal: true

require 'set'
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary as it premises a dependency.

Copy link
Contributor Author

@r7kamura r7kamura Nov 16, 2022

Choose a reason for hiding this comment

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

Thanks, I'll remove this line.

I was wondering about this point as well because this would not only be a matter of necessity, but rather a matter of policy as to who will play the coordinating role, like the contrast between choreography vs. orchestration. I understand that the policy here is to let lib/rubocop.rb in rubocop gem coordinate the dependencies 👌

@koic koic merged commit 43acab1 into rubocop:master Nov 18, 2022
@r7kamura r7kamura deleted the feature/to-s-false branch November 20, 2022 22:29
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

2 participants