-
-
Notifications
You must be signed in to change notification settings - Fork 277
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
Fix RSpec/SortMetadata
cop to sort strings and variables first
#1948
base: master
Are you sure you want to change the base?
Fix RSpec/SortMetadata
cop to sort strings and variables first
#1948
Conversation
it 'registers an offense when a symbol metadata is before second docstring ' \ | ||
'argument' do | ||
expect_offense(<<~RUBY) | ||
RSpec.describe 'Something', :a, :z, 'second docstring' do |
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.
Invalid RSpec syntax:
ArgumentError:
wrong number of arguments (given 4, expected 0..2)
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.
Yes indeed.
Actually there was already an existing test on master
which would lead to this ArgumentError
, even after autocorrect:
rubocop-rspec/spec/rubocop/cop/rspec/sort_metadata_spec.rb
Lines 98 to 111 in 74d7837
it 'registers an offense when using mixed metadata ' \ | |
'and both symbols metadata and hash keys are not in alphabetical order ' \ | |
'and the hash values are complex objects' do | |
expect_offense(<<~RUBY) | |
it 'Something', variable, 'B', :a, key => {}, foo: ->(x) { bar(x) }, Identifier.sample => true, baz: Snafu.new do | |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sort metadata alphabetically. | |
end | |
RUBY | |
expect_correction(<<~RUBY) | |
it 'Something', :a, 'B', variable, baz: Snafu.new, foo: ->(x) { bar(x) }, Identifier.sample => true, key => {} do | |
end | |
RUBY | |
end |
That's why I thought adding this test would be ok. Note that after autocorrect the last string argument is moved at the 2nd place, so the ArgumentError
would not occur anymore.
How should I rewrite this one?
it 'registers an offense when a symbol metadata is before a variable ' \ | ||
'argument' do | ||
expect_offense(<<~RUBY) | ||
RSpec.describe 'Something', :a, :z, variable, foo: :bar do |
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.
If the variable
holds something that is not a symbol, this will fail with a similar ArgumentError
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.
Yes indeed.
It is a bit like this existing test of MetadataStyle
cop:
rubocop-rspec/spec/rubocop/cop/rspec/metadata_style_spec.rb
Lines 354 to 364 in 74d7837
context 'with symbol metadata with another existing non-literal metadata' do | |
it 'registers offense' do | |
expect_offense(<<~RUBY) | |
describe 'Something', :a, b do | |
^^ Use hash style for metadata. | |
end | |
RUBY | |
expect_no_corrections | |
end | |
end |
This one too will fail if b
is not a symbol nor a hash.
How should I rewrite it then? Adding a variable = :v
on the first line?
I suggest extending the spec in accordance with our findings. I'm happy to help on this front. Please accept my apologies for the delay in review, it's a summer season over here. |
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 replied to your comments and I am open to amend this pull request with your suggestions.
Please accept my apologies for the delay as well. As you wrote, it's summer season.
it 'registers an offense when a symbol metadata is before second docstring ' \ | ||
'argument' do | ||
expect_offense(<<~RUBY) | ||
RSpec.describe 'Something', :a, :z, 'second docstring' do |
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.
Yes indeed.
Actually there was already an existing test on master
which would lead to this ArgumentError
, even after autocorrect:
rubocop-rspec/spec/rubocop/cop/rspec/sort_metadata_spec.rb
Lines 98 to 111 in 74d7837
it 'registers an offense when using mixed metadata ' \ | |
'and both symbols metadata and hash keys are not in alphabetical order ' \ | |
'and the hash values are complex objects' do | |
expect_offense(<<~RUBY) | |
it 'Something', variable, 'B', :a, key => {}, foo: ->(x) { bar(x) }, Identifier.sample => true, baz: Snafu.new do | |
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sort metadata alphabetically. | |
end | |
RUBY | |
expect_correction(<<~RUBY) | |
it 'Something', :a, 'B', variable, baz: Snafu.new, foo: ->(x) { bar(x) }, Identifier.sample => true, key => {} do | |
end | |
RUBY | |
end |
That's why I thought adding this test would be ok. Note that after autocorrect the last string argument is moved at the 2nd place, so the ArgumentError
would not occur anymore.
How should I rewrite this one?
it 'registers an offense when a symbol metadata is before a variable ' \ | ||
'argument' do | ||
expect_offense(<<~RUBY) | ||
RSpec.describe 'Something', :a, :z, variable, foo: :bar do |
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.
Yes indeed.
It is a bit like this existing test of MetadataStyle
cop:
rubocop-rspec/spec/rubocop/cop/rspec/metadata_style_spec.rb
Lines 354 to 364 in 74d7837
context 'with symbol metadata with another existing non-literal metadata' do | |
it 'registers offense' do | |
expect_offense(<<~RUBY) | |
describe 'Something', :a, b do | |
^^ Use hash style for metadata. | |
end | |
RUBY | |
expect_no_corrections | |
end | |
end |
This one too will fail if b
is not a symbol nor a hash.
How should I rewrite it then? Adding a variable = :v
on the first line?
95de9d6
to
7d1c8b8
Compare
Hi @pirj, Is there anything I can do to help with this pull request? |
Sorry for the delayed response.
Does this make sense? I’m writing this from my memory, and those criteria may be incomplete or incorrect. Would it be a stretch to ask you to go over existing specs and make sure those criteria are met? If you prefer, I can go through those specs, too. And in any case - thank you for the dedication! |
Yes this makes sense. Not sure if that's exactly what we discussed but this makes sense. Indeed "it should not autocorrect to code that would make RSpec to fail" is what sparked this discussion initially as rubocop-rspec autocorrects this spec file and the resulting corrected file makes RSpec fail because the string parameter is moved from second to third place.
Sure, so I should adapt these specs to ensure that it does not trigger at all, right? I'll do that. Thanks for the feedback. |
621c304
to
9fbf40d
Compare
@pirj I have adapted the spec and code as discussed. It is up for review. It does not completely ignore code that could make RSpec fail. Instead it limits sorting to the trailing args which are actual symbols. Any symbol arg before a string literal or a variable will be ignored. |
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.
Two months later 🙈
'is before a variable argument being the last argument ' \ | ||
'as it could be a hash' do | ||
expect_offense(<<~RUBY) | ||
RSpec.describe 'Something', :z, :a, some_hash do |
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.
Now when I look at some_hash
, did we mean a literal hash? How do we know some_hash
is hash, and not a symbol?
I have no objections to such "chunky" reordering is in place.
Let's split this into two:
- Example with a literal hash
describe 'Something', :z, :a, { foo: 'bar' } do
- Example with many chunks
describe 'Chunks', :z, :a, var, :y, :b, another_var, :x, :c, last_var
The current implementation fixed this to:
describe 'Chunks', :a, another_var, :b, :c, var, :x, :y, :z, last_var do
Which I don't entirely agree with. Variables should retain their positions. Only literal symbols should be rearranged.
Variables not remaining on their places result is a potentially breaking behaviour:
RSpec.describe 'Chunks', second_docstring, :z, :a, var do
RSpec.describe 'Chunks', :a, second_docstring, :z, var do
💥 "wrong number of arguments (given 3, expected 0..2)".
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.
Now when I look at some_hash, did we mean a literal hash? How do we know some_hash is hash, and not a symbol?
We don't know.
- Example with many chunks
describe 'Chunks', :z, :a, var, :y, :b, another_var, :x, :c, last_varThe current implementation fixed this to:
describe 'Chunks', :a, another_var, :b, :c, var, :x, :y, :z, last_var doWhich I don't entirely agree with. Variables should retain their positions. Only literal symbols should be rearranged.
Variables not remaining on their places result is a potentially breaking behaviour:
Yep agreed.
With the implementation proposed in this pull request, it would fix this to:
describe 'Chunks', :z, :a, var, :y, :b, another_var, :c, :x, last_var
only the last symbols part is reordered.
Variables not remaining on their places result is a potentially breaking behaviour:
RSpec.describe 'Chunks', second_docstring, :z, :a, var doRSpec.describe 'Chunks', :a, second_docstring, :z, var do💥 "wrong number of arguments (given 3, expected 0..2)".
With the implementation proposed in this pull request, it would fix this to:
RSpec.describe 'Chunks', second_docstring, :a, :z, var do
That's what triggered me to open this PR.
Metadata processed by RSpec is: - the last argument when it's a hash - the trailing arguments when they are symbols Only this metadata is sorted by this cop. If the second argument to a `context`/`describe` block is used as an additional description, it is not sorted anymore. This fixes rubocop#1946. Co-authored-by: Phil Pirozhkov <[email protected]>
9fbf40d
to
7acdb7c
Compare
Thanks @pirj for the review. I updated it following your suggestions and rebased it on master branch. Is there anything else I should do? |
Fixes #1946.
Symbols in metadata are processed by RSpec only when they are positioned last, meaning the other parameter types must be positioned before the symbols. RSpec
context
/describe
accepts second non-symbol argument as an additional description which is why strings are sorted first.Questions for reviewers:
describe 'Something', :a, b, :c
because the metadata is sorted. I could not come up with a good message. Suggestions?describe 'Something', :b, :a, { foo: :bar }
: it gets autocorrected todescribe 'Something', :b, :a, foo: :bar }
. Should it be fixed in the same PR?RuboCop::Cop::RSpec::Metadata#on_metadata_arguments
method because it was skipping the last argument if it was not a hash. I also renamedsymbols
tometadata_arguments
(orargs
inRSpec/SortMetadata
cop). Shouldsymbols
be renamed tometadata_arguments
orargs
in other cops relying onon_metadata
too?Before 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).