[Fix #647] Add OrderedHooks cop#1119
Conversation
Darhazer
force-pushed
the
ordered-hooks
branch
from
19448e2 to
8521d9d
Compare
pirj
left a comment
pirj
left a comment
There was a problem hiding this comment.
Looks great! Thank you.
And sorry for the delay in reviewing.
|
|
||
| If multiple hooks are defined in example group, they should appear in | ||
| the following order: | ||
| before :suite |
There was a problem hiding this comment.
But before(:suite) cannot appear in an example group? https://github.com/rspec/rspec-core/blob/e95cfe3724de9b56f973e38f84fdb1b609d88e01/lib/rspec/core/hooks.rb#L197
| around :example | ||
| after :example | ||
| after :context | ||
| after :suite |
There was a problem hiding this comment.
| ' at line %<line>d.' | ||
|
|
||
| EXPECTED_HOOK_ORDER = %i[before around after].freeze | ||
| EXPECTED_SCOPE_ORDER = %i[suite context each].freeze |
|
|
||
| RuboCop::RSpec::ExampleGroup.new(node) | ||
| .hooks | ||
| .each_cons(2) { |previous, current| check_order(previous, current) } |
| MSG = '`%<hook>s` is supposed to appear before `%<previous>s`' \ | ||
| ' at line %<line>d.' |
There was a problem hiding this comment.
| MSG = '`%<hook>s` is supposed to appear before `%<previous>s`' \ | |
| ' at line %<line>d.' | |
| MSG = '`%<hook>s` is supposed to appear before `%<previous>s` ' \ | |
| 'at line %<line>d.' |
RuboCop (Parser) unfortunately cannot check this.
| # after { run_cleanup } | ||
| # | ||
| class OrderedHooks < Base | ||
| extend AutoCorrector |
There was a problem hiding this comment.
This can be removed, right?
|
|
||
| | Pending | ||
| | Yes | ||
| | Yes |
There was a problem hiding this comment.
The PR description says there's no autocorrection yet.
| | Yes | |
| | No |
| def format_hook(hook) | ||
| msg = hook.name.to_s | ||
| raw_scope_name = hook.send(:scope_name) | ||
| msg += "(:#{raw_scope_name})" if raw_scope_name |
There was a problem hiding this comment.
Perhaps add the scope only when the hooks are identical? To avoid messages like “before(:each) is supposed to appear before after(:suite)” where I think a message like “before is supposed to appear before after” might be sufficient.
| if current.name == :after # for after we expect reversed order | ||
| order_violation(previous, current) if previous_idx < current_idx | ||
| elsif previous_idx > current_idx | ||
| order_violation(previous, current) | ||
| end |
There was a problem hiding this comment.
I was hoping to get a more symmetric comparison of previous_idx with current_idx. What do you think about e.g.
| if current.name == :after # for after we expect reversed order | |
| order_violation(previous, current) if previous_idx < current_idx | |
| elsif previous_idx > current_idx | |
| order_violation(previous, current) | |
| end | |
| case current.name | |
| when :after # for after we expect reversed order | |
| order_violation(previous, current) if previous_idx < current_idx | |
| else | |
| order_violation(previous, current) if previous_idx > current_idx | |
| end |
|
|
||
| * Fix `HooksBeforeExamples`, `LeadingSubject`, `LetBeforeExamples` and `ScatteredLet` autocorrection to take into account inline comments and comments immediately before the moved node. ([@Darhazer][]) | ||
| * Improve rubocop-rspec performance. ([@Darhazer][]) | ||
| * Add `RSpec/OrderdHooks` cop. ([@Darhazer][]) |
There was a problem hiding this comment.
| * Add `RSpec/OrderdHooks` cop. ([@Darhazer][]) | |
| * Add `RSpec/OrderedHooks` cop. ([@Darhazer][]) |
4 participants
No auto-correction at this point since MoveNode doesn't play nicely with each_cons
Some people may prefer
aroundhooks to be beforebeforehooks, so making it configurable would also be nice.Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif the new code introduces user-observable changes.bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).If you have created a new cop:
config/default.yml.Enabled: pendinginconfig/default.yml.VersionAddedindefault/config.ymlto the next minor version.If you have modified an existing cop's configuration options:
VersionChangedinconfig/default.ymlto the next major version.