Add abort to the Rails/Exit cop
#1537
Conversation
adamstegman
force-pushed
the
rails-exit-abort
branch
2 times, most recently
from
f49fff4 to
5690b95
Compare
lib/rubocop/cop/rails/exit.rb
Outdated
|
|
||
| MSG = 'Do not use `exit` in Rails applications.' | ||
| RESTRICT_ON_SEND = %i[exit exit!].freeze | ||
| MSG = 'Do not use `exit` or `abort` in Rails applications.' |
There was a problem hiding this comment.
It might be clearer to limit it to methods that are actually used, for example as shown below.
The test code should also be updated accordingly to reflect this change.
diff --git a/lib/rubocop/cop/rails/exit.rb b/lib/rubocop/cop/rails/exit.rb
index fd1901970..5517f1454 100644
--- a/lib/rubocop/cop/rails/exit.rb
+++ b/lib/rubocop/cop/rails/exit.rb
@@ -26,12 +26,16 @@ module RuboCop
class Exit < Base
include ConfigurableEnforcedStyle
- MSG = 'Do not use `exit` or `abort` in Rails applications.'
+ MSG = 'Do not use `%<current>s` in Rails applications.'
RESTRICT_ON_SEND = %i[exit exit! abort].freeze
EXPLICIT_RECEIVERS = %i[Kernel Process].freeze
def on_send(node)
- add_offense(node.loc.selector) if offending_node?(node)
+ return unless offending_node?(node)
+
+ message = format(MSG, current: node.method_name)
+
+ add_offense(node.loc.selector, message: message)
end
privateThere was a problem hiding this comment.
Thank you for the pointer to how to accomplish this! I've updated the PR with this suggestion.
`abort` is very similar to `exit` in Ruby ([docs](https://docs.ruby-lang.org/en/3.4/Kernel.html#method-i-abort)), but it takes a string message or Exception. Rails applications should avoid its usage for the same reasons as they avoid `exit` and `exit!`. Since they are used in the same way, I thought it belonged in the same cop, but am glad to move it to a new one if that's preferred.
adamstegman
force-pushed
the
rails-exit-abort
branch
from
5690b95 to
c1f5d98
Compare
|
Thanks! |
|
Curious about this rule RE: a scenario like "we need certain ENV vars to be present and set a certain way to function properly". Using Curious what the recommendation would be for that "detecting stuff during app boot". Should this cop ignore |
|
@mjankowski I think |
|
Yeah - I think for developer-facing environment/config issues doing a swap over to For packaged/distributed apps where you potentially have admins/operators (who are not devs and may not be sure what to do with a backtrace, but who DO understand exit codes and console warnings), it's less ideal. Maybe that's an edge case and the disable is fine. Agreed on point re: inline vs full directory to not let any sneak through. |
3 participants
abortis very similar toexitin Ruby (docs), but it takes a string message or Exception. Rails applications should avoid its usage for the same reasons as they avoidexitandexit!. Since they are used in the same way, I thought it belonged in the same cop, but am glad to move it to a new one if that's preferred.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.