RSpec/SpecFilePathFormat allow multiple values for same key in IgnoreMetadata by sriedel · Pull Request #2070 · rubocop/rubocop-rspec

sriedel · 2025-04-04T10:51:27Z

RSpec/SpecFilePathFormat allows ignoring certain values in rspec metadata through the IgnoreMetadata option, as given in the documentation:

      # @example `IgnoreMetadata: {type=>routing}` (default)
      #   # good
      #   whatever_spec.rb         # describe MyClass, type: :routing do; end

However the current implementation does not seem to allow supplying multiple values for the same key - e.g. if you have some specs marked type: controller, others as type: :model etc, only one of controller, model, ... can be supplied to be ignored.

This is probably due to an oversight (or me not being able to figure out how to properly supply an array of values to be ignored).

The PR allows for either an array or a single value to be given for a key in the IgnoreMetadata option.

While it would also be desirable to give a true value to ignore all metadata, in the format that IgnoreMethods use, this should probably be part of a different PR.

…/SpecFilePathFormat

bquorning · 2025-04-04T11:42:21Z

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


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.

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.

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?

ydah · 2025-04-04T13:35:04Z

  IgnoreMetadata:
    type: routing
  VersionAdded: '2.24'
+  VersionChanged: "<<next>>"


This change is unnecessary. Could you please revert?

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.

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

Fix a InternalAffairs/NodeTypeGroup

Fixes: #1943

bquorning · 2025-04-04T14:52:27Z

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


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

ydah · 2025-04-04T14:57:16Z

Could you rebase to the latest master branch?

Change `RSpec/ScatteredSetup` to allow `around` hooks to be scattered

issue number: #2069

Fix an error `RSpec/ChangeByZero` cop when without expect block

…/SpecFilePathFormat

…e_metadata' of github.com:sriedel/rubocop-rspec into sr/allow_multiple_values_to_spec_file_path_format_ignore_metadata

pirj · 2025-04-07T09:29:03Z

      #   # good
      #   whatever_spec.rb         # describe MyClass, type: :routing do; end
      #
+      # @example `IgnoreMetadata: {type=>[routing,models]}` (default)


Is it really configured like this in YAML?

pirj · 2025-04-07T09:30:00Z

+      # @example `IgnoreMetadata: {type=>[routing,models]}` (default)
+      #   # good
+      #   whatever_spec.rb         # describe MyClass, type: :routing do; end
+      #   whatever_spec.rb         # describe MyClass, type: :models do; end


What if the metadata's value IS an array?

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

How do we define IgnoreMetadata in YAML?

pirj · 2025-04-07T09:31:19Z

        private

        def register_offense(node, change_node)
+          return unless node.parent.send_type?


pirj · 2025-04-07T09:31:37Z


        def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler
-          return if node.each_ancestor(:def, :defs).any?
+          return if node.each_ancestor(:any_def).any?


This, too, and related specs

pirj · 2025-04-07T09:32:30Z

+      # @example `IgnoreMetadata: {type=>[routing,models]}` (default)
+      #   # good
+      #   whatever_spec.rb         # describe MyClass, type: :routing do; end
+      #   whatever_spec.rb         # describe MyClass, type: :models do; end


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

pirj · 2025-04-07T09:38:31Z

+      # @example `IgnoreMetadata: {type=>[routing,models]}` (default)
+      #   # good
+      #   whatever_spec.rb         # describe MyClass, type: :routing do; end
+      #   whatever_spec.rb         # describe MyClass, type: :models do; end


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.

sriedel · 2025-04-07T14:55:20Z

I seem to have fat-fingered the rebase. I'll close this PR and reopen a new one with the current state of changes tomorrow.

sriedel · 2025-04-10T09:21:30Z

Clean rebase PR in #2076

Allow array of values to be given for ignoring metadata tags in RSpec…

cd98181

…/SpecFilePathFormat

sriedel requested a review from a team as a code owner April 4, 2025 10:51

bquorning reviewed Apr 4, 2025

View reviewed changes

Sven Riedel and others added 2 commits April 4, 2025 14:18

Extend specs to be more comprehensive for IgnoreMetadata evaluation

055b97b

Fix a InternalAffairs/NodeTypeGroup

0816629

ydah reviewed Apr 4, 2025

View reviewed changes

ydah added 2 commits April 4, 2025 22:36

Merge pull request #2072 from rubocop/fix-offenses

0f0fb8b

Fix a InternalAffairs/NodeTypeGroup

Change RSpec/ScatteredSetup to allow around hooks to be scattered

bfe68bc

Fixes: #1943

bquorning approved these changes Apr 4, 2025

View reviewed changes

ydah and others added 10 commits April 5, 2025 00:10

Merge pull request #2073 from rubocop/1943

14f3dd2

Change `RSpec/ScatteredSetup` to allow `around` hooks to be scattered

Fix RSpec/ChangeByZero cop to handle invalid change matcher usage

8c4429e

issue number: #2069

Merge pull request #2071 from lee266/bugfix-change-by-zero

c25abf4

Fix an error `RSpec/ChangeByZero` cop when without expect block

Prefer Array() over explicit array wrapping with conditional

7bf9936

Revert VersionChanged attribute for RSpec/SpecFilePathFormat

ce6d89a

Allow array of values to be given for ignoring metadata tags in RSpec…

62d2662

…/SpecFilePathFormat

Extend specs to be more comprehensive for IgnoreMetadata evaluation

d59185b

Prefer Array() over explicit array wrapping with conditional

cd2f7b0

Revert VersionChanged attribute for RSpec/SpecFilePathFormat

71de1e8

Merge branch 'sr/allow_multiple_values_to_spec_file_path_format_ignor…

8bcbbf3

…e_metadata' of github.com:sriedel/rubocop-rspec into sr/allow_multiple_values_to_spec_file_path_format_ignore_metadata

pirj reviewed Apr 7, 2025

View reviewed changes

Comment thread CHANGELOG.md Outdated

Update CHANGELOG.md

473104b

pirj reviewed Apr 7, 2025

View reviewed changes

sriedel mentioned this pull request Apr 10, 2025

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

Closed

sriedel closed this Apr 10, 2025

pirj mentioned this pull request Apr 29, 2025

Allow ignoring by multiple metadata entries in RSpec/SpecFilePathFormat #2084

Open

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

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

Uh oh!

Uh oh!

Conversation

sriedel commented Apr 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydah Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydah commented Apr 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sriedel commented Apr 7, 2025

Uh oh!

sriedel commented Apr 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ydah Apr 4, 2025 •

edited

Loading