-
-
Notifications
You must be signed in to change notification settings - Fork 266
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
Add Safety
section to documentation
#575
Conversation
2035d0b
to
e059e3f
Compare
lib/rubocop/cop/rails/save_bang.rb
Outdated
# This cop's autocorrection is unsafe because custom `update` method call was changed to `update!` | ||
# but the method name remained same in the method definition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# This cop's autocorrection is unsafe because custom `update` method call was changed to `update!` | |
# but the method name remained same in the method definition. | |
# This cop's autocorrection is unsafe because a custom `update` method call would be changed to `update!`, | |
# but the method name in the definition would be unchanged. |
@@ -5,6 +5,10 @@ module Cop | |||
module Rails | |||
# This cop checks that controllers subclass ApplicationController. | |||
# | |||
# @safety | |||
# This cop's autocorrection is unsafe because a controller class which inherits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble understanding what this means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pirj's comment text (#575 (comment)) is excellent and I updated it.
@@ -7,6 +7,10 @@ module Rails | |||
# Use `find_by` instead of dynamic method. | |||
# See. https://rails.rubystyle.guide#find_by | |||
# | |||
# @safety | |||
# This cop is unsafe because it may detect user-defined `find_by_xxx` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# This cop is unsafe because it may detect user-defined `find_by_xxx` method. | |
# This cop is unsafe because it may detect a user-defined `find_by_xxx` method. |
lib/rubocop/cop/rails/mailer_name.rb
Outdated
@@ -8,6 +8,10 @@ module Rails | |||
# Without the `Mailer` suffix it isn't immediately apparent what's a mailer | |||
# and which views are related to the mailer. | |||
# | |||
# @safety | |||
# This cop's autocorrection is unsafe because Renaming a constant is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# This cop's autocorrection is unsafe because Renaming a constant is | |
# This cop's autocorrection is unsafe because renaming a constant is |
# a receiver object that do not have `exclude?` method. (e.g. `IPAddr`) | ||
# @safety | ||
# This cop is unsafe because false positive will occur for | ||
# a receiver object that do not have `exclude?` method. (e.g. `IPAddr`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# a receiver object that do not have `exclude?` method. (e.g. `IPAddr`) | |
# receiver objects that do not have an `exclude?` method. (e.g. `IPAddr`) |
lib/rubocop/cop/rails/pick.rb
Outdated
# This cop is unsafe because pluck is defined on both `ActiveRecord::Relation` and `Enumerable`, | ||
# whereas `pick` is only defined on `ActiveRecord::Relation` in Rails 6.0. This will be addressed | ||
# in Rails 6.1 via rails/rails#38760, at which point the cop will be safe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# This cop is unsafe because pluck is defined on both `ActiveRecord::Relation` and `Enumerable`, | |
# whereas `pick` is only defined on `ActiveRecord::Relation` in Rails 6.0. This will be addressed | |
# in Rails 6.1 via rails/rails#38760, at which point the cop will be safe. | |
# This cop is unsafe because `pluck` is defined on both `ActiveRecord::Relation` and `Enumerable`, | |
# whereas `pick` is only defined on `ActiveRecord::Relation` in Rails 6.0. This was addressed | |
# in Rails 6.1 via rails/rails#38760, at which point the cop is safe. |
@@ -14,6 +14,10 @@ module Rails | |||
# * The task does not need application code. | |||
# * The task invokes the `:environment` task. | |||
# | |||
# @safety | |||
# Probably not a problem in most cases, but it is likely that calling `:environment` task |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Probably not a problem in most cases, but it is likely that calling `:environment` task | |
# Probably not a problem in most cases, but it is possible that calling `:environment` task |
(otherwise it seems like the sentence is a contradiction).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredible job!
# @safety | ||
# This cop is unsafe because custom `update_attributes` method call was changed to | ||
# `update` but the method name remained same in the method definition. | ||
# | ||
# @example | ||
# #bad | ||
# Book.update_attributes!(author: 'Alice') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated: I believe there's a typo here, should be lowercase book
.
lib/rubocop/cop/rails/arel_star.rb
Outdated
# [source,ruby] | ||
# --- | ||
# hash = { "*" => true } | ||
# hash["*"] # This would get corrected to `hash[Arel.star]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is from a "does not register an offense" example.
Here is a comment that explains why we've kept the cop unsafe.
WDYT of:
This cop's autocorrection is unsafe because it turns a quoted
*
into an SQL*
, unquoted.*
is a valid column name in certain databases supported by Rails, and even though it is usually a mistake, it might denote legitimate access to a column named*
.
# This cop's autocorrection is unsafe because a controller class which inherits | ||
# `ActionController::Base` directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and similar job/mailer/model)'s descriptions are hard to understand.
Maybe something like:
This cop's autocorrection is unsafe because it may let the logic from
ApplicationContoller
sneak into a controller that is not purposed to inherit logic common among other controllers.
@@ -7,6 +7,10 @@ module Rails | |||
# Use `find_by` instead of dynamic method. | |||
# See. https://rails.rubystyle.guide#find_by | |||
# | |||
# @safety | |||
# This cop is unsafe because it may detect user-defined `find_by_xxx` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that the cop is unsafe. It is certainly unsafe when not configured properly, i.e. user-defined find_by_xxx
method is not added to cop's AllowedMethods
.
lib/rubocop/cop/rails/mailer_name.rb
Outdated
@@ -8,6 +8,10 @@ module Rails | |||
# Without the `Mailer` suffix it isn't immediately apparent what's a mailer | |||
# and which views are related to the mailer. | |||
# | |||
# @safety | |||
# This cop's autocorrection is unsafe because Renaming a constant is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic: lowercase "Renaming"
# Probably not a problem in most cases, but it is likely that calling `:environment` task | ||
# will break a behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also slower. E.g. some task that only needs one gem to be loaded to run will run significantly faster without loading the whole application.
# This cop is unsafe because it cannot be determined whether | ||
# constant or method return value specified to `class_name` is a string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like in MODEL_CLASS_REGISTRY.names['account']
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anything I need to add about this?
# | ||
# [source,ruby] | ||
# ---- | ||
# foo&.blank? #=> nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that someone would write this intentionally?
I would argue that the cop is unsafe for flagging this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on the following.
https://github.com/rubocop/rubocop-rails/blob/v2.12.3/config/default.yml#L712-L718
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes total sense to me to draw attention, but not autocorrect 👍
My apologies, I've missed the point - thought that it's Safe: false
, not SafeAutoCorrect: false
.
(I lack permissions to resolve this thread).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that it's
Safe: false
, notSafeAutoCorrect: false
.
Sure! I've opened #577.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion, I meant that its current config is fine. I mistakenly thought it is Safe: false
, so I was worried that those running rubocop --safe
would miss its offences.
lib/rubocop/cop/rails/save_bang.rb
Outdated
# This cop's autocorrection is unsafe because custom `update` method call was changed to `update!` | ||
# but the method name remained same in the method definition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain in a little more detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the example code of #98.
lib/rubocop/cop/rails/time_zone.rb
Outdated
# @safety | ||
# This cop is unsafe because it may change handling time. | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. It's a pity it's marked as unsafe. Do you think it's possible to make it autocorrect-unsafe instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! It makes sense to me! This does not seem to cause false positives. I look rubocop/rubocop#6966, which should have used SafeAutoCorrect: false
correctly. So I've opened PR #576 (I adjust the text of the @safety
for the next release) .
Follow up to rubocop#575 (comment) This PR marks `Rails/TimeZone` as unsafe auto-correction from unsafe.
@andyw8 @pirj Thank you for the many great reviews! I've updated most of it except for #575 (comment) and #575 (comment). |
e9ac684
to
a1e8986
Compare
Follow up to rubocop/rubocop#10094. This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.
a1e8986
to
26d71af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you!
Follow up to rubocop/rubocop#10094.
This PR adds safety section to documentation for all cops that are
Safe: false
orSafeAutoCorrect: false
.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.and description in grammatically correct, complete sentences.
bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.