Skip to content

Autocorrect safely with RuboCop#53138

Closed
schmijos wants to merge 1 commit intorails:mainfrom
schmijos:safe-autocorrect
Closed

Autocorrect safely with RuboCop#53138
schmijos wants to merge 1 commit intorails:mainfrom
schmijos:safe-autocorrect

Conversation

@schmijos
Copy link
Contributor

@schmijos schmijos commented Oct 1, 2024

rubocop -A would generate false positive fixes.
After this change we use rubocop -a instead.
The idea is not to change semantics with RuboCop
See https://docs.rubocop.org/rubocop/usage/auto_correct.html#safe-auto-correct

`rubocop -A` would generate false positives.
After this change we use `rubocop -a` instead.
See https://docs.rubocop.org/rubocop/usage/auto_correct.html#safe-auto-correct
@koic
Copy link
Contributor

koic commented Oct 1, 2024

In the generated code, no use cases that would cause false positives are expected. Therefore, -A is intentionally specified. When Style/FrozenStringLiteralComment cop is enabled, -A ensures the insertion of the fstring magic comment, which is often overlooked. This is the most compelling use case for specifying -A instead of -a.

What specific use cases would cause issues with -A when using bin/rails g?

@schmijos
Copy link
Contributor Author

schmijos commented Oct 1, 2024

My concern is not with specific use cases coming from Rails itself (code and cops under control of rails/rails) but it's one about resilience with either generated files from other gems or cops from adjusted configurations.

For me running rubocop -a is a good default. But running rubocop -A is always a conscious decision whose results I carefully review.

@Earlopain
Copy link
Contributor

The new rails authentication framework, while created through a generator, is handwritten code being copied into a users own app. The authentication-zero gem has even more code being installed like that. I checked and it only creates safe offenses (with rubocop-rails-omakase) but I don't think it's possible to assume that generated code will always be simple.

It's highly unfortunate that Style/FrozenStringLiteralComment must be unsafe and I will celebrate the day when it is not needed anymore.

@andyw8
Copy link
Contributor

andyw8 commented Oct 1, 2024

One way around the Style/FrozenStringLiteralComment issue is to mark it as safe, even though it technically isn't, as the Shopify styleguide does:

https://github.com/Shopify/ruby-style-guide/blob/72773041ac9c03e252d0ce66a2c8424d320be21e/rubocop.yml#L554-L555

@yahonda
Copy link
Member

yahonda commented Dec 13, 2024

Thank you for opening a pull request. After thinking about this, I've decided to close this pull request.

Here are reasons why: Applying unsafe or safe auto correct depends not only on the command-line options -A or -a, but also on whether the user's rubocop.yml contains unsafe cops. So, if users always want autocorrect to use only safe cops, they can define a rubocop.yml that contains only safe cops.

As for unsafe cops, Some people want Style/FrozenStringLiteralComment cop to add # frozen_string_literal: true to files created by a generator. I can assume that the user accepts this insecurity in such cases.

Thanks again for opening a pull request. Looking forward to future contributions.

@yahonda yahonda closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants