Fix `RSpec/SortMetadata` cop to sort only trailing metadata #1948

cbliard · 2024-08-12T06:37:58Z

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.

Previously this cop was sorting all arguments, regardless of their type, which could lead to having the second string argument (the additional description) moved to another place (see #1946 for details). This PR fixes it.

Questions for reviewers:

~~is it ok to sort strings before variables? I have the feeling that this cop's responsibility should be to just take care of sorting symbols at the end of the arguments list.~~ As discussed during the review, only the last metadata arguments are considered (symbols and final hash argument).
the message "Sort metadata alphabetically." does not fully reflect what the cop is doing anymore, and can be confusing for code like describe 'Something', :a, b, :c because the metadata is sorted. I could not come up with a good message. Suggestions? As only the trailing metadata is sorted now, the message seems ok.
I noticed an autocorrect issue with describe 'Something', :b, :a, { foo: :bar }: it gets autocorrected to describe 'Something', :b, :a, foo: :bar }. Should it be fixed in the same PR?
I updated the RuboCop::Cop::RSpec::Metadata#on_metadata_arguments method because it was skipping the last argument if it was not a hash. I also renamed symbols to metadata_arguments (or args in RSpec/SortMetadata cop). Should symbols be renamed to metadata_arguments or args in other cops relying on on_metadata too?

Before submitting the PR make sure the following are checked:

Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Updated documentation.
Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

pirj · 2024-08-17T07:36:52Z

spec/rubocop/cop/rspec/sort_metadata_spec.rb

+  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


Invalid RSpec syntax:

ArgumentError: wrong number of arguments (given 4, expected 0..2)

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?

pirj · 2024-08-17T07:39:19Z

spec/rubocop/cop/rspec/sort_metadata_spec.rb

+  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


If the variable holds something that is not a symbol, this will fail with a similar ArgumentError

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?

spec/rubocop/cop/rspec/sort_metadata_spec.rb

pirj · 2024-08-17T09:54:30Z

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.

cbliard

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.

cbliard · 2024-08-28T14:22:12Z

spec/rubocop/cop/rspec/sort_metadata_spec.rb

+  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


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?

cbliard · 2024-08-28T14:27:14Z

spec/rubocop/cop/rspec/sort_metadata_spec.rb

+  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


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?

spec/rubocop/cop/rspec/sort_metadata_spec.rb

cbliard · 2024-09-25T13:42:05Z

Hi @pirj,

Is there anything I can do to help with this pull request?

pirj · 2024-10-08T16:46:13Z

Sorry for the delayed response.
We had a fruitful discussion in the ticket about what cop should and shouldn’t do. I recall it as:

it should not attempt to handle code that will make RSpec fail anyway
it should handle realistic code
it should not autocorrect to code that would make RSpec to fail (was it what sparked this discussion initially?)
it should consider those subtle differences between examples and example groups when it comes to metadata arguments
it should ignore non-literal metadata arguments, and avoid risks automatically ordering them

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?
Last time I checked, I was under impression that a few cases in the very beginning of the changes to specs were off, and this repelled me from code reviewing further, as I thought it would require a lot commenting.

If you prefer, I can go through those specs, too.

And in any case - thank you for the dedication!
This is doable, let’s push the cop through the line where it can shine! 🌟

cbliard · 2024-10-09T15:27:38Z

We had a fruitful discussion in the ticket about what cop should and shouldn’t do. I recall it as:

it should not attempt to handle code that will make RSpec fail anyway

it should handle realistic code

it should not autocorrect to code that would make RSpec to fail (was it what sparked this discussion initially?)

it should consider those subtle differences between examples and example groups when it comes to metadata arguments

it should ignore non-literal metadata arguments, and avoid risks automatically ordering them

Does this make sense? I’m writing this from my memory, and those criteria may be incomplete or incorrect.

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.

Would it be a stretch to ask you to go over existing specs and make sure those criteria are met? Last time I checked, I was under impression that a few cases in the very beginning of the changes to specs were off, and this repelled me from code reviewing further, as I thought it would require a lot commenting.

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.

cbliard · 2024-10-10T07:41:59Z

@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.

pirj

Two months later 🙈

spec/rubocop/cop/rspec/sort_metadata_spec.rb

cbliard · 2024-12-23T15:57:57Z

Thanks @pirj for the review. I updated it following your suggestions and rebased it on master branch.

Is there anything else I should do?

pirj

Thank you for the immense effort you've put into this!

Please accept my apologies for the daunting experience with those ages-long reviews. It is usually not like this in our repos 🙈

bquorning · 2025-01-06T06:30:26Z

spec/rubocop/cop/rspec/sort_metadata_spec.rb

+  it 'registers an offense when a symbol metadata not in alphabetical order ' \
+     'is before a variable argument being the last argument ' \
+     'as it could be a hash' do


This seems a bit strange to me. If some_hash “could be a hash”, it could also be e.g. an array. And if we sort symbol metadata before an array variable, why don’t we always sort symbol metadata before a variable?

That's a valid point. Indeed each group of symbols could be alphabetically sorted independently.

This PR was originally addressing #1946 and is now approved. If that makes sense, could your point be registered as a separate issue to be addressed later? Feel free to mention me and I can work on a PR for it.

My comment was not meant to block this PR; I’m find with merging this as-is.

cbliard · 2025-01-06T08:36:31Z

Thank you @pirj for the review.

No worries for the time it took. I was not very available as well.

cbliard · 2025-01-09T12:16:27Z

Anything particular that can be done to make the "CI / Prism" check pass and get that PR merged?

ydah · 2025-01-09T12:21:58Z

Anything particular that can be done to make the "CI / Prism" check pass and get that PR merged?

@cbliard Could you try git rebase and import the latest master branch?

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]>

cbliard · 2025-01-09T15:05:12Z

Thanks @ydah, that fix the Prism issue thanks to the example producing a syntax error you removed 2 weeks ago.

Then there was this error poping on "Edge RuboCop: internal_investigation" workflow:

lib/rubocop/rspec/description_extractor.rb:65:11: C: [Correctable] Style/MultipleComparison: Avoid comparing a variable with multiple items in a conditional, use Array#include? instead.
          yardoc.superclass.path == RSPEC_COP_CLASS_NAME || ...
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I fixed it too and force pushed again.

pirj · 2025-01-09T21:03:23Z

Thank you! 🙌🌟

cbliard requested a review from a team as a code owner August 12, 2024 06:37

pirj reviewed Aug 17, 2024

View reviewed changes

spec/rubocop/cop/rspec/sort_metadata_spec.rb Outdated Show resolved Hide resolved

pirj reviewed Aug 17, 2024

View reviewed changes

spec/rubocop/cop/rspec/sort_metadata_spec.rb Outdated Show resolved Hide resolved

cbliard commented Aug 28, 2024

View reviewed changes

cbliard force-pushed the sort_metadata_symbols_at_the_end_of_args_list branch from 95de9d6 to 7d1c8b8 Compare September 11, 2024 06:17

cbliard force-pushed the sort_metadata_symbols_at_the_end_of_args_list branch 2 times, most recently from 621c304 to 9fbf40d Compare October 10, 2024 07:24

cbliard requested a review from pirj October 14, 2024 06:41

pirj reviewed Dec 9, 2024

View reviewed changes

cbliard force-pushed the sort_metadata_symbols_at_the_end_of_args_list branch from 9fbf40d to 7acdb7c Compare December 23, 2024 15:48

cbliard changed the title ~~Fix RSpec/SortMetadata cop to sort strings and variables first~~ Fix RSpec/SortMetadata cop to sort only trailing metadata Dec 30, 2024

pirj approved these changes Jan 6, 2025

View reviewed changes

pirj requested a review from a team January 6, 2025 06:08

bquorning approved these changes Jan 6, 2025

View reviewed changes

cbliard force-pushed the sort_metadata_symbols_at_the_end_of_args_list branch from 7acdb7c to 0ba1569 Compare January 9, 2025 14:39

cbliard force-pushed the sort_metadata_symbols_at_the_end_of_args_list branch from 0ba1569 to c2e861f Compare January 9, 2025 15:03

pirj merged commit e44a27b into rubocop:master Jan 9, 2025
27 checks passed

cbliard deleted the sort_metadata_symbols_at_the_end_of_args_list branch January 10, 2025 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `RSpec/SortMetadata` cop to sort only trailing metadata #1948

Fix `RSpec/SortMetadata` cop to sort only trailing metadata #1948

cbliard commented Aug 12, 2024 •

edited

Loading

pirj Aug 17, 2024

cbliard Aug 28, 2024

pirj Aug 17, 2024

cbliard Aug 28, 2024

pirj commented Aug 17, 2024

cbliard left a comment

cbliard Aug 28, 2024

cbliard Aug 28, 2024

cbliard commented Sep 25, 2024

pirj commented Oct 8, 2024

cbliard commented Oct 9, 2024

cbliard commented Oct 10, 2024

pirj left a comment

cbliard commented Dec 23, 2024

pirj left a comment

bquorning Jan 6, 2025

cbliard Jan 6, 2025

bquorning Jan 6, 2025

cbliard commented Jan 6, 2025

cbliard commented Jan 9, 2025

ydah commented Jan 9, 2025

cbliard commented Jan 9, 2025

pirj commented Jan 9, 2025

	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

	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

Fix RSpec/SortMetadata cop to sort only trailing metadata #1948

Fix RSpec/SortMetadata cop to sort only trailing metadata #1948

Conversation

cbliard commented Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj commented Aug 17, 2024

cbliard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbliard commented Sep 25, 2024

pirj commented Oct 8, 2024

cbliard commented Oct 9, 2024

cbliard commented Oct 10, 2024

pirj left a comment

Choose a reason for hiding this comment

cbliard commented Dec 23, 2024

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbliard commented Jan 6, 2025

cbliard commented Jan 9, 2025

ydah commented Jan 9, 2025

cbliard commented Jan 9, 2025

pirj commented Jan 9, 2025

Fix `RSpec/SortMetadata` cop to sort only trailing metadata #1948

Fix `RSpec/SortMetadata` cop to sort only trailing metadata #1948

cbliard commented Aug 12, 2024 •

edited

Loading