-
-
Notifications
You must be signed in to change notification settings - Fork 281
Support itblock
#2077
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
base: master
Are you sure you want to change the base?
Support itblock
#2077
Conversation
return unless example_group_with_body?(node) | ||
|
||
check_hooks(node.body) if multiline_block?(node.body) | ||
end | ||
|
||
alias on_numblock on_block |
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 had no effect, so I removed it. It relies on a previous block and then simply uses next_sibling
together with any_block
(which includes itblock
). I only added tests to show it already works.
@@ -38,7 +38,6 @@ def on_block(node) | |||
on_metadata_arguments(metadata_arguments) | |||
end | |||
end | |||
alias on_numblock on_block |
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 module is included in 4 cops and none have tests for numblocks. So I simply removed this one.
I don't think handling these blocks makes sense in context anyways and it was only added here because of the internal affairs cop
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 think it would be helpful to have some guidelines on when to add numblock/itblock aliases, and when it does not. It's too easy to appease toe internal affairs cop and just add a useless alias.
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.
In this case it's for describe
/context
. You could handle numblocks but what would taking a block argument even do for those functions? There are a few other cases like this that I can think of like prepend
/include
/extend
that take a block but never any arguments.
I guess as long as the block actually takes an argument. If a use can write foo { |arg| }
and have the code make sense, then the same with numblock/itblock should also be possible.
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 recall we had a PR where we were adding aliases everywhere, and then we just disabled that cop, and decided not to merge the pr. I yet to see any missed offence caused by lack of those aliases to justify their addition.
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 can see how that cop can be annoying. Personally I never use numblocks (and I think I'm not the only one, just doesn't look very nice) but I do actually write it
.
@@ -39,5 +39,5 @@ Gem::Specification.new do |spec| | |||
} | |||
|
|||
spec.add_dependency 'lint_roller', '~> 1.1' | |||
spec.add_dependency 'rubocop', '~> 1.72', '>= 1.72.1' | |||
spec.add_dependency 'rubocop', '~> 1.75' |
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.
1.75 transitively depends on rubocop-ast >= 1.43.0
which is good enough
I guess I should clarify: |
Hm, coverage is reduced because when using the |
|
||
context 'when Ruby 3.4', :ruby34, unsupported_on: :parser do | ||
it 'does not flag :example for hooks' do | ||
expect_no_offenses(<<~RUBY) |
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 change this to ‘it.run’ not to offend the other cop?
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 copy-pasted the current numblock specs, will change these as well.
expect_offense(<<~RUBY) | ||
RSpec.describe User do | ||
it { is_expected.to be_after_around_hook } | ||
around { it } |
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 wonder if this would work anyway without making a special case for an it-block? Looks like a regular no-args block call with an ‘it’ call inside. No surprise, this may or may not be an it-block, hard to tell.
Side note, how does Ruby knows ‘describe { it { foo } }’ is not an it-block?
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.
By what I propose here to keep the spec, and attempt to remove special handling in the cop. Chances are it will work anyway.
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.
Yup, that's what I did: #2077 (comment)
Side note, how does Ruby knows ‘describe { it { foo } }’ is not an it-block?
it
with block had special considerations from ruby itself, it
only points to the block argument when it
doesn't take a block itself. https://bugs.ruby-lang.org/issues/18980
The following cases have been discussed:
- it method, most famously in RSpec: You almost always pass a positional and/or block argument to RSpec's it, so the conflict is avoided with my proposal. You virtually never use a completely naked it
e4a4818
to
3279266
Compare
ba0f9df
to
89a75bf
Compare
Coverage is no longer an issue (rubocop/rubocop#14119) but now there's a gnarly-looking ruby-head failure I'm 100% certain is not my fault (: (https://bugs.ruby-lang.org/issues/21357) |
89a75bf
to
1dfb00c
Compare
Friendly ping |
All cops that handle numblock now also handle itblock. `itblock` is relatively new and the required `rubocop-ast` version is required via RuboCop 1.75
1dfb00c
to
c34a0c1
Compare
All cops that handle numblock now also handle itblock.
itblock
is relatively new and the requiredrubocop-ast
version is required via RuboCop 1.75Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if 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).