Fix `RSpec/SortMetadata` cop to sort strings and variables first

[cbliard]

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:

• 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.
• 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?
• 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:

• [x] Feature branch is up-to-date with master (if not - rebase it).
• [x] Squashed related commits together.
• [x] Added tests.
• [ ] Updated documentation.
• [x] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
• [x] 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]

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]

Hi @pirj,

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

[pirj]

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]

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]

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

[cbliard]

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

Is there anything else I should do?

[cbliard]

Thank you @pirj for the review.

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

[cbliard]

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

[ydah]

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?

[cbliard]

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]

Thank you! 🙌🌟