Skip to content
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

Jump to bottom

Split RSpec/FilePath into RSpec/SpecFilePathSuffix and RSpec/SpecFilePathFormat #1698

Merged
merged 4 commits into from
Aug 26, 2023
Merged

Split RSpec/FilePath into RSpec/SpecFilePathSuffix and RSpec/SpecFilePathFormat #1698

merged 4 commits into from
Aug 26, 2023

Conversation

ydah
Copy link
Member

@ydah ydah commented Aug 16, 2023

Fix: #1069

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.

pirj
pirj reviewed Aug 17, 2023
View reviewed changes
lib/rubocop/cop/rspec/spec_file_path_format.rb
return unless top_level_groups.one?

example_group_arguments(node) do |class_name, arguments|
next if !class_name.const_type? || ignore_metadata?(arguments)
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the PR. It seems we’re completely ignoring class names in the string format.
RSpec.describe ‘MyClass’ do
Also, Would ‘const_type?’ return true for cbase?

pirj
pirj reviewed Aug 17, 2023
View reviewed changes
lib/rubocop/cop/rspec/spec_file_path_format.rb
add_global_offense(format(MSG, suffix: suffix))
end

def ignore_metadata?(arguments)
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to the PR.
I think it might be possible to use a trmplated node pattern instead of the two nested loops.

pirj
pirj reviewed Aug 17, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pirj
pirj approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Perfect, thank you so much!
Just one cosmetic note regarding parameter naming, and a few notes that we can turn into issues to work on in the future.

@ydah ydah force-pushed the split-file-path-cop branch 2 times, most recently from fc3d2d5 to 2ecf181 Compare August 18, 2023 09:38
@ydah ydah requested a review from pirj August 18, 2023 09:47
pirj
pirj approved these changes Aug 18, 2023
View reviewed changes
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Perfect, thank you!

@pirj pirj mentioned this pull request Aug 18, 2023
Open
bquorning
bquorning reviewed Aug 20, 2023
View reviewed changes
lib/rubocop/cop/rspec/spec_file_path_format.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/spec_file_path_format.rb Outdated Show resolved Hide resolved
lib/rubocop/cop/rspec/spec_file_path_format.rb Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -3,6 +3,7 @@
## Master (Unreleased)

- Fix an infinite loop error when `RSpec/ExcessiveDocstringSpacing` finds a description with non-ASCII leading/trailing whitespace. ([@bcgraham])
- Split `RSpec/FilePath` into `RSpec/SpecFilePathSuffix` and `RSpec/SpecFilePathFormat`. ([@ydah])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we remind people here that while the old RSpec/FilePath cop was enabled by default, the two new cops are pending and need to be explicitly enabled?

@ydah ydah force-pushed the split-file-path-cop branch 2 times, most recently from 21507cf to 6dd86e0 Compare August 25, 2023 15:25
@ydah
Copy link
Member Author

ydah commented Aug 25, 2023

@bquorning Sorry for the delay. I updated this PR.

@ydah ydah requested a review from bquorning August 25, 2023 15:27
bquorning
bquorning approved these changes Aug 26, 2023
View reviewed changes
Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you.

@bquorning bquorning merged commit de0b715 into master Aug 26, 2023
22 checks passed
@bquorning bquorning deleted the split-file-path-cop branch August 26, 2023 11:41
mvz added a commit to troessner/reek that referenced this pull request Sep 8, 2023
@mvz
Update configuration due to RSpec/FilePath cop split
44d64dc 
See rubocop/rubocop-rspec#1698.
@ydah ydah mentioned this pull request Sep 11, 2023
Closed
5 tasks
splattael
splattael reviewed Sep 12, 2023
View reviewed changes
config/default.yml
@@ -442,7 +442,7 @@ RSpec/ExpectOutput:

RSpec/FilePath:
Description: Checks that spec file paths are consistent and well-formed.
Enabled: true
Enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: The changelog reads that RSpec/FilePath is "enabled by default" but it seems it isn't anymore.

Currently, users are forced to replace RSpec/FilePath with their alternatives when upgrading a minor release from 2.23.0 to 2.24.0. Is this expected or do we consider this a breaking change? 🤔

Refs https://gitlab.com/gitlab-org/ruby/gems/gitlab-styles/-/merge_requests/195

/cc @ydah @bquorning

Copy link
Contributor

Choose a reason for hiding this comment

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

I can open an issue to continue the discussion if needed ❤️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please open an issue. I am not sure what is the preferred course of action – should we add the old cop back?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ydah @bquorning I've moved the discussion to #1717.

@splattael splattael mentioned this pull request Sep 12, 2023
Closed
@granth granth mentioned this pull request Sep 19, 2023
Merged
granth added a commit to 4ormat/styleguide that referenced this pull request Sep 19, 2023
@granth
split RSpec/SpecFilePath (#27)
4600323 
`RSpec/SpecFilePath` was split into `RSpec/SpecFilePathFormat` and `RSpec/SpecFilePathSuffix` in `rubocop-rspec` 2.24.0.

rubocop/rubocop-rspec#1698

The old cop was enabled by default but the new ones are not 🤷

See 4ormat/4ormat#17161
ydah added a commit that referenced this pull request Feb 16, 2024
@ydah
Add test case for RSpec/SpecFilePathFormat
e615663 
Follow up: #1698 (comment)
@ydah ydah mentioned this pull request Feb 16, 2024
Merged
4 tasks
ydah added a commit that referenced this pull request Feb 16, 2024
@ydah
Add test case for RSpec/SpecFilePathFormat
c0c3e04 
Follow up: #1698 (comment)
@nishidayuya nishidayuya mentioned this pull request Jun 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@splattael splattael splattael left review comments

@bquorning bquorning bquorning approved these changes

@pirj pirj pirj approved these changes

@Darhazer Darhazer Awaiting requested review from Darhazer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RSpec/FilePath cop is completely skipped on routing specs, or specs that don't describe a constant
4 participants
@ydah @pirj @bquorning @splattael