Fix an error when using `RSpec/FilePath` and revert to enabled by default #1718

ydah · 2023-09-12T09:20:36Z

Fix: #1717

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.

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

Set VersionChanged: "<<next>>" in config/default.yml.

ydah · 2023-09-12T09:32:45Z

It appears that Danger is failing because GITHUB_TOKEN does not have write permission.
I don't have permission to see the Settings in this repository, so could someone please check if it is set as follows?

https://github.com/rubocop/rubocop-rspec/settings/actions

pirj

Thank you!

config/obsoletion.yml

docs/modules/ROOT/pages/cops_rspec.adoc

bquorning · 2023-09-12T20:17:49Z

It appears that Danger is failing because GITHUB_TOKEN does not have write permission.

On the surface, that seems a bit dangerous. Isn’t is possible to use a token with fewer permissions.

pirj · 2023-09-13T07:24:16Z

Is Danger trying to tell us that we’re introducing a cop in Enabled status?

wondering if there’s an option to tell danger not to post PR comments but rather just dump their warnings to stdout for inspection

splattael · 2023-09-13T11:25:26Z

CHANGELOG.md

 ## 2.24.0 (2023-09-08)

- Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. `RSpec/FilePath` cop is enabled by default, the two new cops are pending and need to be enabled explicitly. ([@ydah])
+- **(Breaking)** Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. `RSpec/FilePath` cop is disabled by default and the two new cops are pending and need to be enabled explicitly. ([@ydah])


I am confused. Isn't RSpec/FilePath now enabled by default again? 🤔 If so, is this really a breaking change then? 🤓

Suggested change

- **(Breaking)** Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. `RSpec/FilePath` cop is disabled by default and the two new cops are pending and need to be enabled explicitly. ([@ydah])

- Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. `RSpec/FilePath` cop is enabled by default and the two new cops are pending and need to be enabled explicitly. ([@ydah])

Oh, sure, we'll enable it back for next version, but isn't it is breaking change for those who use v2.24.0?

Perhaps we’re asking “does two breaking changes equal zero breaking changes”?

Ah, I understand. Indeed it is.

I updated this PR. How about it?

Feedback: Our builds were actually broken twice by this two breaking changes.

ydah · 2023-09-14T09:58:35Z

Is Danger trying to tell us that we’re introducing a cop in Enabled status?

wondering if there’s an option to tell danger not to post PR comments but rather just dump their warnings to stdout for inspection

https://danger.systems/ruby/
No functionality seems to be provided for output to stdout.

pirj · 2023-09-14T10:44:55Z

I suggest replacing Danger with a shell script file. We only use it to grep through the PR diff, should be doable, right?

ydah · 2023-09-15T02:27:05Z

@rubocop/rubocop-rspec Danger replacement should be able to be done as a follow-up, so it would be nice to be able to merge and do a patch version release if there are no problems with this first. How about it?

pirj

Thank you!

CHANGELOG.md

pirj

Now when I think of this - why have we disabled the cop? Should we enable it again for those who don’t follow the brave ‘NewCops: true’ path? Can we split and remove the old cop in a major version, and meanwhile just keep those two new pending cops?
New cops would duplicate the function of the old one, but this is not a problem, is it?

ydah · 2023-09-20T06:33:44Z

I had misunderstood the comment I received at #1718 (comment) . I changed it to ~~Enabled: false~~ Enabled: true and changed the migration plan for CHANGELOG. @bquorning @pirj How about this?

pirj · 2023-09-20T07:13:38Z

Just for the reference #1717 (comment)

i don’t think we need to disable FilePath until v3 (where we remove it).

config/default.yml

pirj

Thank you!

CHANGELOG.md

…ault

This PR commits `RSpec/FilePath` to suppress the following offenses: ```console $ bundle exec rake internal_investigation (snip) Offenses: spec/rubocop/cop/internal_affairs/redundant_let_rubocop_config_new_spec.rb:3:1: C: RSpec/FilePath: Spec path should end with rubocop/cop/internal_affairs/redundant_let_rubo_cop_config_new*_spec.rb. RSpec.describe RuboCop::Cop::InternalAffairs::RedundantLetRuboCopConfigNew, :config do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/rubocop/cop/mixin/enforce_superclass_spec.rb:3:1: C: RSpec/FilePath: Spec path should end with rubocop/cop/enforce_superclass*_spec.rb. RSpec.describe RuboCop::Cop::EnforceSuperclass, :restore_registry do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ spec/rubocop/formatter/junit_formatter_spec.rb:3:1: C: RSpec/FilePath: Spec path should end with rubocop/formatter/j_unit_formatter*_spec.rb. RSpec.describe RuboCop::Formatter::JUnitFormatter, :config do ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ``` https://app.circleci.com/pipelines/github/rubocop/rubocop/9942/workflows/0c732f10-f59d-45a9-be69-5af5ceb0c5d8/jobs/289057 The offenses are due to a change in RuboCop RSpec 2.24.1, and it will ignore the deprecated `RSpec/FilePath`. rubocop/rubocop-rspec#1718

timon · 2023-09-25T12:00:02Z

I think I see regression with this and custom inflections in Zeitwerk.
rubocop-rspec 2.24.1 fails CI for our project using customer inflections:

# config/initializers/zeitwerk.rb
Rails.autoloaders.each do |autoloader|
  autoloader.inflector = Zeitwerk::Inflector.new
  autoloader.inflector.inflect(
    'bvd' => 'BvD'
  )
end

Every spec file that covers BvD module now registers an offense like this:

spec/services/bvd/adapters/fetch_data_spec.rb:5:1: C: RSpec/FilePath: Spec path should end with bv_d/adapters/fetch_data*_spec.rb.
describe BvD::Adapters::FetchData do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It was not an issue neither for 2.24.0, nor for 2.23.2 (although I'm not sure if it was skipped)

AlexWayfer · 2023-09-25T12:24:11Z

@timon I see no info that rubocop-rspec can support Zeitwerk inflections for it's own configuration.

I can suggest to you to add some configuration.

You've got the warning from RSpec/FilePath. Let's see its documentation: https://docs.rubocop.org/rubocop-rspec/cops_rspec.html#rspecfilepath

CustomTransform option looks exactly what do you need.

domon-envato · 2023-09-28T01:59:09Z

lib/rubocop/cop/rspec/file_path.rb

+      # We plan to remove it in the next major version update to 3.0.
+      # The migration targets are `RSpec/SpecFilePathSuffix`
+      # and `RSpec/SpecFilePathFormat`.
+      # If you are using this cop, please plan for migration.


@ydah What is the suggested way to migrate?

I've updated my config like the following for 0.24.0.

Removed RSpec/FilePath

Disabled RSpec/SpecFilePathFormat

Enabled RSpec/SpecFilePathSuffix

But this new change in 0.24.1 is forcing me to update my config again like the following.

Add RSpec/FilePath back and disable it

Does that mean my config will be broken when RSpec/FilePath is removed in the next major version update to 3.0?

I understand your frustration with some reconfiguration required for a minor/patch level version dependency updates.

May I kindly ask you to elaborate what you mean by "forcing"?

You meant 2.24.0.

my config will be broken ... in the next major version update to 3.0?

Major updates often come with breaking changes, so most probably yes.
But worry not, we'll provide a detailed update checklist like we did with 1.x -> 2.x update.

Thanks for responding to my question 🙏

May I kindly ask you to elaborate what you mean by "forcing"?
You meant 2.24.0.

Before 2.24.0: I had RSpec/FilePath explicitly disabled in my config.

At 2.24.0: I had to remove RSpec/FilePath completely from my config or the rubocop command would not exit with a successful status.

At 2.24.1: Now I had to disable RSpec/FilePath again or it would be enabled by default.

After updated the config twice in each of these two versions, I was a bit alerted by the words "plan for migration" and curious about what that actually means.

But on second thought, I think 2.24.1 just moved the state back to what we were at before 2.24.0.

If I interpret what you said correctly, there will be no breaking changes like this until 3.0. But we probably will need to update the config when upgrading to 3.x, which is quite normal. Is my understanding correct?

I see, thanks for the explanation. And please accept my apologies for introducing breaking changes.

Your understanding is correct.

ydah force-pushed the fix-braking branch from 6ccfe5d to d4152b7 Compare September 12, 2023 09:24

pirj approved these changes Sep 12, 2023

View reviewed changes

config/obsoletion.yml Show resolved Hide resolved

docs/modules/ROOT/pages/cops_rspec.adoc Outdated Show resolved Hide resolved

ydah force-pushed the fix-braking branch from d4152b7 to 1571a44 Compare September 13, 2023 05:22

splattael reviewed Sep 13, 2023

View reviewed changes

ydah force-pushed the fix-braking branch from 1571a44 to 5f3d793 Compare September 13, 2023 12:06

splattael mentioned this pull request Sep 14, 2023

RSpec/FilePath split is disabled by default #1717

Closed

ydah requested a review from bquorning September 14, 2023 09:58

ydah requested review from pirj and splattael September 15, 2023 02:27

pirj approved these changes Sep 15, 2023

View reviewed changes

bquorning reviewed Sep 16, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ydah force-pushed the fix-braking branch from 5f3d793 to b7ac3f1 Compare September 18, 2023 06:54

ydah requested a review from bquorning September 18, 2023 06:55

ydah changed the title ~~Fix an error when using RSpec/FilePath and revert to enabled by default~~ Fix an error when using RSpec/FilePath Sep 18, 2023

ydah requested review from a team and removed request for splattael September 20, 2023 01:40

pirj reviewed Sep 20, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ydah force-pushed the fix-braking branch from b7ac3f1 to 9dedd1c Compare September 20, 2023 06:15

pirj reviewed Sep 20, 2023

View reviewed changes

bquorning mentioned this pull request Sep 20, 2023

Complete the deprecation of RSpec/FilePath #1723

Closed

ydah force-pushed the fix-braking branch from 9dedd1c to ec35bab Compare September 20, 2023 06:28

ydah changed the title ~~Fix an error when using RSpec/FilePath~~ Fix an error when using RSpec/FilePath and revert to enabled by default. Sep 20, 2023

ydah force-pushed the fix-braking branch from ec35bab to 195a87f Compare September 20, 2023 06:30

ydah changed the title ~~Fix an error when using RSpec/FilePath and revert to enabled by default.~~ Fix an error when using RSpec/FilePath and revert to enabled by default Sep 20, 2023

ydah force-pushed the fix-braking branch from 195a87f to 1357be4 Compare September 20, 2023 06:31

ydah commented Sep 20, 2023

View reviewed changes

config/default.yml Show resolved Hide resolved

ydah requested a review from pirj September 20, 2023 07:42

pirj approved these changes Sep 21, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

bquorning approved these changes Sep 22, 2023

View reviewed changes

Fix an error when using RSpec/FilePath and revert to enabled by def…

c3d53d9

…ault

ydah force-pushed the fix-braking branch from b7833cf to c3d53d9 Compare September 23, 2023 01:41

ydah requested a review from pirj September 23, 2023 01:42

pirj approved these changes Sep 23, 2023

View reviewed changes

bquorning merged commit e7d0fd5 into master Sep 23, 2023
21 of 22 checks passed

bquorning deleted the fix-braking branch September 23, 2023 08:00

domon-envato reviewed Sep 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix an error when using `RSpec/FilePath` and revert to enabled by default #1718

Fix an error when using `RSpec/FilePath` and revert to enabled by default #1718

ydah commented Sep 12, 2023 •

edited

Loading

ydah commented Sep 12, 2023

pirj left a comment

bquorning commented Sep 12, 2023

pirj commented Sep 13, 2023

splattael Sep 13, 2023

ydah Sep 13, 2023 •

edited

Loading

bquorning Sep 13, 2023

ydah Sep 13, 2023

ydah Sep 15, 2023

domon-envato Sep 28, 2023

ydah commented Sep 14, 2023

pirj commented Sep 14, 2023

ydah commented Sep 15, 2023

pirj left a comment •

edited

Loading

pirj left a comment

ydah commented Sep 20, 2023 •

edited

Loading

pirj commented Sep 20, 2023

pirj left a comment

timon commented Sep 25, 2023

AlexWayfer commented Sep 25, 2023

domon-envato Sep 28, 2023

pirj Sep 28, 2023

domon-envato Sep 28, 2023

pirj Sep 28, 2023

	- (Breaking) Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. `RSpec/FilePath` cop is disabled by default and the two new cops are pending and need to be enabled explicitly. ([@ydah])
	- Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. `RSpec/FilePath` cop is enabled by default and the two new cops are pending and need to be enabled explicitly. ([@ydah])

Fix an error when using RSpec/FilePath and revert to enabled by default #1718

Fix an error when using RSpec/FilePath and revert to enabled by default #1718

Conversation

ydah commented Sep 12, 2023 • edited Loading

ydah commented Sep 12, 2023

pirj left a comment

Choose a reason for hiding this comment

bquorning commented Sep 12, 2023

pirj commented Sep 13, 2023

Choose a reason for hiding this comment

ydah Sep 13, 2023 • 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

Choose a reason for hiding this comment

ydah commented Sep 14, 2023

pirj commented Sep 14, 2023

ydah commented Sep 15, 2023

pirj left a comment • edited Loading

Choose a reason for hiding this comment

pirj left a comment

Choose a reason for hiding this comment

ydah commented Sep 20, 2023 • edited Loading

pirj commented Sep 20, 2023

pirj left a comment

Choose a reason for hiding this comment

timon commented Sep 25, 2023

AlexWayfer commented Sep 25, 2023

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

Fix an error when using `RSpec/FilePath` and revert to enabled by default #1718

Fix an error when using `RSpec/FilePath` and revert to enabled by default #1718

ydah commented Sep 12, 2023 •

edited

Loading

ydah Sep 13, 2023 •

edited

Loading

pirj left a comment •

edited

Loading

ydah commented Sep 20, 2023 •

edited

Loading