Skip to content

RSpec/SpecFilePathFormat allow multiple values for same key in IgnoreMetadata #2070

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.

Sign up for GitHub

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

Closed
sriedel wants to merge 16 commits into from
Closed

RSpec/SpecFilePathFormat allow multiple values for same key in IgnoreMetadata #2070

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
cd98181
Allow array of values to be given for ignoring metadata tags in RSpec…
Apr 4, 2025
055b97b
Extend specs to be more comprehensive for IgnoreMetadata evaluation
Apr 4, 2025
0816629
Fix a InternalAffairs/NodeTypeGroup
ydah Apr 4, 2025
0f0fb8b
Merge pull request #2072 from rubocop/fix-offenses
ydah Apr 4, 2025
bfe68bc
Change `RSpec/ScatteredSetup` to allow `around` hooks to be scattered
ydah Apr 4, 2025
14f3dd2
Merge pull request #2073 from rubocop/1943
ydah Apr 4, 2025
8c4429e
Fix RSpec/ChangeByZero cop to handle invalid change matcher usage
Apr 4, 2025
c25abf4
Merge pull request #2071 from lee266/bugfix-change-by-zero
pirj Apr 5, 2025
7bf9936
Prefer Array() over explicit array wrapping with conditional
Apr 7, 2025
ce6d89a
Revert VersionChanged attribute for RSpec/SpecFilePathFormat
Apr 7, 2025
62d2662
Allow array of values to be given for ignoring metadata tags in RSpec…
Apr 4, 2025
d59185b
Extend specs to be more comprehensive for IgnoreMetadata evaluation
Apr 4, 2025
cd2f7b0
Prefer Array() over explicit array wrapping with conditional
Apr 7, 2025
71de1e8
Revert VersionChanged attribute for RSpec/SpecFilePathFormat
Apr 7, 2025
8bcbbf3
Merge branch 'sr/allow_multiple_values_to_spec_file_path_format_ignor…
Apr 7, 2025
473104b
Update CHANGELOG.md
pirj Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/default.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,7 @@ RSpec/SpecFilePathFormat:
IgnoreMetadata:
type: routing
VersionAdded: '2.24'
VersionChanged: "<<next>>"

@ydah ydah Apr 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unnecessary. Could you please revert?

@sriedel sriedel Apr 4, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, however I'd like to confirm that the revert would be correct, as the Pull Request template specifically states:

If you have modified an existing cop's configuration options:

- [ ] Set `VersionChanged: "<<next>>"` in `config/default.yml`.

and this PR does change an existing cops configuration options.

@ydah ydah Apr 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to my understanding, "modified an existing cop's configuration options" refers to instances such as when adding a new configuration option, changing a name, or for example, when adding autocorrect functionality and adding Autocorrect: true, we should add VersionChanged: "<<next>>".

Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/SpecFilePathFormat

RSpec/SpecFilePathSuffix:
Expand Down
10 changes: 9 additions & 1 deletion lib/rubocop/cop/rspec/spec_file_path_format.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ module RSpec
# # good
# whatever_spec.rb # describe MyClass, type: :routing do; end
#
# @example `IgnoreMetadata: {type=>[routing,models]}` (default)

@pirj pirj Apr 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really configured like this in YAML?

# # good
# whatever_spec.rb # describe MyClass, type: :routing do; end
# whatever_spec.rb # describe MyClass, type: :models do; end

@pirj pirj Apr 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the metadata's value IS an array?

RSpec.describe Airplane, prepare: [:fuel] do

How do we define IgnoreMetadata in YAML?

@pirj pirj Apr 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: how do we ignore any value for a key?

@pirj pirj Apr 7, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hinting towards:

IgnoreMetadata:
  - prepare
  - type: model
  - type: routing
  - skip: [database]

Luckily enough, YAML allows to mix all that, and to cover all ignored cases unambiguously.

#
class SpecFilePathFormat < Base
include TopLevelGroup
include Namespace
Expand Down Expand Up @@ -73,7 +78,10 @@ def ensure_correct_file_path(send_node, class_name, arguments)
def ignore_metadata?(arguments)
arguments.any? do |argument|
metadata_key_value(argument).any? do |key, value|
ignore_metadata.values_at(key.to_s).include?(value.to_s)
ignore_values = ignore_metadata[key.to_s]
ignore_values = [ignore_values] unless ignore_values.is_a?(Array)

@bquorning bquorning Apr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use Array() instead:

Suggested change
ignore_values = ignore_metadata[key.to_s]
ignore_values = [ignore_values] unless ignore_values.is_a?(Array)
ignore_values = Array(ignore_metadata[key.to_s])


ignore_values.include?(value.to_s)
end
end
end
Expand Down
10 changes: 10 additions & 0 deletions spec/rubocop/cop/rspec/spec_file_path_format_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,4 +281,14 @@ class Foo
RUBY
end
end

context 'when configured with `IgnoreMetadata: { "foo" => ["bar"] }`' do

@bquorning bquorning Apr 4, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe you should write a spec for IgnoreMetadata: { "foo" => ["bar", "baz"] } and validate that it ignores specs with either value. And that it does not ignore a spec without another value for foo, or no foo metadata at all.

@sriedel sriedel Apr 4, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, are the amended specs what you had in mind? I needed to play around with the wording a bit, since I was over the line length limit by one and two characters respectively, by using the existing phrasing.

@pirj pirj Apr 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for ignorance, but what if the value of the metdata value is an array of bar baz?
Or an array of just bar?

I haven’t glanced over the changes, but if we happen to want to allow multiple values, there will be less ambiguity if we accept accept a list of metadata to ignore. Will it work?

let(:cop_config) { { 'IgnoreMetadata' => { 'foo' => ['bar'] } } }

it 'does not register an offense when include ignored metadata' do
expect_no_offenses(<<~RUBY, 'wrong_class_spec.rb')
describe MyClass, foo: :bar do; end
RUBY
end
end
end