Add RSpec/MetadataStyle and RSpec/EmptyMetadata cops

[r7kamura]

This Pull Request implements the new Cop discussed in the following Pull Request:

• Add RSpec/DuplicatedMetadata cop #1486 (comment)

Besides just the benefit of a consistent look and feel of code, this Cop would allow us to autocorrect the following code duplication:

# 1
describe 'Something', :config, config: true

# 2 (autocorrected by RSpec/MetadataStyle)
describe 'Something', :config, :config

# 3 (autocorrected by RSpec/DuplicatedMetadata)
describe 'Something', :config

---

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

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop is configured as Enabled: pending in config/default.yml.
• The cop is configured as Enabled: true in .rubocop.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.
• Set VersionAdded: "<<next>>" in default/config.yml.

[pirj]

What is the case exactly?

[r7kamura]

If you try to autocorrect code like this:

hash = { config: true }
describe 'Something', :a, hash

you will probably get the following result (and this code probably won't work well):

hash = { config: true }
describe 'Something', hash, a: true

---

That's what I thought at first, but now that I've written the test, I see that RSpec::Cop::RSpec::Metadata#on_block is not working as intended 🤔

context 'with symbol metadata with another existing non-literal metadata' do
  it 'registers offense' do
    expect_offense(<<~RUBY)
      hash = { config: true }
      describe 'Something', :a, hash do
                            ^^ Use hash style for metadata.
      end
    RUBY

    expect_correction(<<~RUBY)
      hash = { config: true }
      describe 'Something', hash, a: true do
      end
    RUBY
  end
end

$ bundle exec rspec spec/rubocop/cop/rspec/metadata_style_spec.rb
Run options: include {:focus=>true}

Randomized with seed 20276

RuboCop::Cop::RSpec::MetadataStyle
  with `EnforcedStyle: hash`
    with symbol metadata with another existing non-literal metadata
      registers offense (FAILED - 1)

Failures:

  1) RuboCop::Cop::RSpec::MetadataStyle with `EnforcedStyle: hash` with symbol metadata with another existing non-literal metadata registers offense
     Failure/Error: super
     
       Diff:
       @@ -1,5 +1,4 @@
        hash = { config: true }
        describe 'Something', :a, hash do
       -                      ^^ Use hash style for metadata.
        end
       
     # ./spec/support/expect_offense.rb:17:in `expect_offense'
     # ./spec/rubocop/cop/rspec/metadata_style_spec.rb:215:in `block (4 levels) in <top (required)>'

Finished in 0.02385 seconds (files took 0.59292 seconds to load)
1 example, 1 failure

Failed examples:

rspec ./spec/rubocop/cop/rspec/metadata_style_spec.rb:214 # RuboCop::Cop::RSpec::MetadataStyle with `EnforcedStyle: hash` with symbol metadata with another existing non-literal metadata registers offense

We might want to improve this matcher:

rubocop-rspec/lib/rubocop/cop/rspec/mixin/metadata.rb

Lines 26 to 28 in 2aea1be

	def_node_search :metadata_in_block, <<~PATTERN
	(send (lvar %) #Hooks.all _ ${send str sym}* (hash $...)?)
	PATTERN

[pirj]

Indeed, Metadata only works with literal metadata.
Maybe we should keep it this way?
Then, we can make this cop autocorrect safe.
I'd add a test that this case with a var is ignored.

[r7kamura]

The reasons why I think this matcher can be improved are:

• We can consider any non-trailing metadata as Symbol metadata even if it is not literal
• Not really relevant to this issue, but there is no need to support str node here

How about marking the autocorrection as safe by saying that it is not supported if the tailing argument is neither a Hash nor Symbol literal?

That way, users can at least know that there is an offense in that case like this:

describe 'Something', :a, b do
                      ^^ Use hash style for metadata.
end

I know that this kind of case is quite rare, but one might argue that the use of Symbol metadata should be allowed in this case. Personally, I prefer to consistently assume that this is an offense as it is simpler and easier to understand.

[Darhazer]

Once again, sorry for the delay in the reviews.

Overall this looks good. Perhaps you can bring it up to date and enrich the specs 🙏 🙇

[Darhazer]

Suggested change

	symbols = metadata_arguments[0..-2]
	pairs = []
	last = metadata_arguments.last
	*symbols, last = metadata_arguments
	pairs = []

[Darhazer]

aren't we missing spec that mixes both preceding symbol metadata, and having other hash metadata?
What about having 2 hash keys with values true?

[r7kamura]

Sorry for the delay. I was adding tests and found a bug around autocorrection that left unnecessary commas and empty braces, so I fixed a lot of things.

In addition, since yesterday, CI seemed to be failing for other reasons, so I created the following pull request.

• Auto-correct InternalAffairs/LocationExpression offenses #1586

I'm still having trouble with CI failing due to decreased coverage, but it doesn't look like I've added any new untested code 🤔

[pirj]

Force-pushed to make ruby-head green.

[pirj]

CI failing due to decreased coverage, but it doesn't look like I've added any new untested code 🤔

The only reasonable complaint is that those lines are not covered: https://github.com/rubocop/rubocop-rspec/pull/1497/files#diff-cbd3200503c5c2c8e9b3c2cdc44ee465010609fdbbe49908cdd278da67c9ad1aR84-R85

can it be the culprit?

[r7kamura]

Sorry, I was mistaken about the coverage report. Indeed, a condition was written in if that would always be true, and the else side had not been tested!

diff --git a/lib/rubocop/cop/rspec/metadata_style.rb b/lib/rubocop/cop/rspec/metadata_style.rb
index 768ea074..84b31b14 100644
--- a/lib/rubocop/cop/rspec/metadata_style.rb
+++ b/lib/rubocop/cop/rspec/metadata_style.rb
@@ -79,11 +79,7 @@ module RuboCop
         end
 
         def format_symbol_to_pair_source(node)
-          if node.sym_type?
-            "#{node.value}: true"
-          else
-            "#{node.source} => true"
-          end
+          "#{node.value}: true"
         end
 
         def insert_pair(corrector, node)

[Darhazer]

@r7kamura can you bring this up to date 🙏
I'll do my best for a timely review this time

[Darhazer]

Great work. Thank you 🙇

[r7kamura]

Thanks, I added test cases for a: false and "a" => true 👍

[ydah]

Looks good. Thank you!

[pirj]

I have limited capacity for reviews now. I trust your judgement guys, please proceed at your discretion.
@r7kamura Thank you for another contribution!

[ydah]

Looks good! Thank you!

[Darhazer]

@r7kamura thank you 🙇 🎉