-
-
Notifications
You must be signed in to change notification settings - Fork 276
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
NestedGroups counts levels differently since version 1.9.0 #270
Comments
Thanks for the report @mvz I think you're right I'll take a look |
backus
added a commit
that referenced
this issue
Jan 2, 2017
The changes in bd88c07 changes the node pattern for NestedGroups from (block (send nil [SELECTORS UNION PATTERN] ...) (args) ...) to (block (send _ [SELECTORS UNION PATTERN] ...) ...) which led to issue #270 being reported. While this did introduce a change in behavior, it helped reveal an inconsistency in the implementation. With the change mentioned above, the following code generated an offense when it didn't in 1.8.0 RSpec.describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end the following nearly equivalent code *did* generate an offense in 1.8 though: describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end The problem was that the previous node pattern would only start counting nesting at the first bare `describe` or `context` and would not count `RSpec.describe`. It was also probably much less efficient since it was searching through all nodes inside every example group instead of only searching from top level describes. This change fixes #270 by starting a search at the top level describe and properly handle `RSpec.describe` like `describe`. This change also convinced me that the current default for NestedGroups is too aggressive. Almost all of the specs that I write use `RSpec.describe` which means that rubocop-rspec was more lenient for my tests. The default MaxNesting is now 3 instead of 2.
backus
added a commit
that referenced
this issue
Jan 2, 2017
The changes in bd88c07 changes the node pattern for NestedGroups from (block (send nil [SELECTORS UNION PATTERN] ...) (args) ...) to (block (send _ [SELECTORS UNION PATTERN] ...) ...) which led to issue #270 being reported. While this did introduce a change in behavior, it helped reveal an inconsistency in the implementation. With the change mentioned above, the following code generated an offense when it didn't in 1.8.0 RSpec.describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end the following nearly equivalent code *did* generate an offense in 1.8 though: describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end The problem was that the previous node pattern would only start counting nesting at the first bare `describe` or `context` and would not count `RSpec.describe`. It was also probably much less efficient since it was searching through all nodes inside every example group instead of only searching from top level describes. This change fixes #270 by starting a search at the top level describe and properly handle `RSpec.describe` like `describe`. This change also convinced me that the current default for NestedGroups is too aggressive. Almost all of the specs that I write use `RSpec.describe` which means that rubocop-rspec was more lenient for my tests. The default MaxNesting is now 3 instead of 2.
backus
added a commit
that referenced
this issue
Jan 2, 2017
The changes in bd88c07 changes the node pattern for NestedGroups from (block (send nil [SELECTORS UNION PATTERN] ...) (args) ...) to (block (send _ [SELECTORS UNION PATTERN] ...) ...) which led to issue #270 being reported. While this did introduce a change in behavior, it helped reveal an inconsistency in the implementation. With the change mentioned above, the following code generated an offense when it didn't in 1.8.0 RSpec.describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end the following nearly equivalent code *did* generate an offense in 1.8 though: describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end The problem was that the previous node pattern would only start counting nesting at the first bare `describe` or `context` and would not count `RSpec.describe`. It was also probably much less efficient since it was searching through all nodes inside every example group instead of only searching from top level describes. This change fixes #270 by starting a search at the top level describe and properly handle `RSpec.describe` like `describe`. This change also convinced me that the current default for NestedGroups is too aggressive. Almost all of the specs that I write use `RSpec.describe` which means that rubocop-rspec was more lenient for my tests. The default MaxNesting is now 3 instead of 2.
backus
added a commit
that referenced
this issue
Jan 2, 2017
The changes in bd88c07 changes the node pattern for NestedGroups from (block (send nil [SELECTORS UNION PATTERN] ...) (args) ...) to (block (send _ [SELECTORS UNION PATTERN] ...) ...) which led to issue #270 being reported. While this did introduce a change in behavior, it helped reveal an inconsistency in the implementation. With the change mentioned above, the following code generated an offense when it didn't in 1.8.0 RSpec.describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end the following nearly equivalent code *did* generate an offense in 1.8 though: describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end The problem was that the previous node pattern would only start counting nesting at the first bare `describe` or `context` and would not count `RSpec.describe`. It was also probably much less efficient since it was searching through all nodes inside every example group instead of only searching from top level describes. This change fixes #270 by starting a search at the top level describe and properly handle `RSpec.describe` like `describe`. This change also convinced me that the current default for NestedGroups is too aggressive. Almost all of the specs that I write use `RSpec.describe` which means that rubocop-rspec was more lenient for my tests. The default MaxNesting is now 3 instead of 2.
Thanks, @backus! |
@backus sure, hold on. |
Thanks :) |
Looks good, @backus. |
Thanks @mvz |
markburns
pushed a commit
to markburns/rubocop-rails-ddd
that referenced
this issue
Nov 7, 2017
The changes in bd88c07 changes the node pattern for NestedGroups from (block (send nil [SELECTORS UNION PATTERN] ...) (args) ...) to (block (send _ [SELECTORS UNION PATTERN] ...) ...) which led to issue rubocop#270 being reported. While this did introduce a change in behavior, it helped reveal an inconsistency in the implementation. With the change mentioned above, the following code generated an offense when it didn't in 1.8.0 RSpec.describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end the following nearly equivalent code *did* generate an offense in 1.8 though: describe Foo do context 'bar' do it 'baz' do expect(foo).to_not be(1) expect(foo).not_to be(1) end end describe '#qux' do context 'when quuz' do it 'zyxxy' do pass end end end end The problem was that the previous node pattern would only start counting nesting at the first bare `describe` or `context` and would not count `RSpec.describe`. It was also probably much less efficient since it was searching through all nodes inside every example group instead of only searching from top level describes. This change fixes rubocop#270 by starting a search at the top level describe and properly handle `RSpec.describe` like `describe`. This change also convinced me that the current default for NestedGroups is too aggressive. Almost all of the specs that I write use `RSpec.describe` which means that rubocop-rspec was more lenient for my tests. The default MaxNesting is now 3 instead of 2.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
With version 1.8.0, the following does not cause a NestedGroups offense:
With 1.9.0, it seems the nested is counted differently, since it registers an offense, even thought the MaxNesting parameter default has stayed the same, at
2
.This seems to be a bug or regressions, since this changes is not marked in the change log.
The text was updated successfully, but these errors were encountered: