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
Allow autocorrect with display-only-fail-level-offenses #12535
Conversation
0024ae9
to
6416464
Compare
668c50b
to
23e65ab
Compare
it 'fails if given with an autocorrect argument' do | ||
%w[--fix-layout -x --autocorrect -a --autocorrect-all -A].each do |o| | ||
expect { options.parse ['--display-only-correctable', o] } | ||
.to raise_error(RuboCop::OptionArgumentError) |
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 seems that instead of being removed, it should be updated to reflect an expected behavior, rather than resulting in the RuboCop::OptionArgumentError
error.
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 not sure I follow. This tests the validation that I removed in order to allow the two arguments to be used together. I could change it to not_to
expect the error, but did you have something else in mind?
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.
Yeah, for now, the test should probably be changed to not_to
.
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.
Done. The test was actually not doing the right thing before, because it was re-using the same Options
instance and generating errors for a different reason (using -a and -A together)
The `--display-only-fail-level-offenses` option uses `considered_failure?` to determine what to print. The same method is used to determine the exit code of `rubocop`, so the two options should be entirely safe to use together. This change is useful when using rubocop as part of a pre-commit hook. When the hook fails becuase rubocop exited with a non-zero status, we can use `--display-only-fail-level-offenses` to avoid having the output cluttered up with non-blocking offenses.
23e65ab
to
b695ff7
Compare
@koic would appreciate another look when you have a chance, thanks! 🙏 |
The change looks good to me. Thanks! |
The
--display-only-fail-level-offenses
option usesconsidered_failure?
to determine what to print. The same method isused to determine the exit code of
rubocop
, so the two options shouldbe entirely safe to use together.
This change is useful when using rubocop as part of a pre-commit hook.
When the hook fails becuase rubocop exited with a non-zero status, we
can use
--display-only-fail-level-offenses
to avoid having the outputcluttered up with non-blocking offenses.
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}.md
if the new code introduces user-observable changes. See changelog entry format for details.