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

Add new RSpec/RemoveConst cop #1749

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Add new RSpec/RemoveConst cop #1749

merged 1 commit into from
Dec 14, 2023

Conversation

swelther
Copy link
Contributor

@swelther swelther commented Dec 1, 2023 •
edited
Loading

This PR adds a cop that forbids the usage of Object.remove_const in specs. We had some strange errors and side effect because of this usage in our project and wanted to get rid of the usage in specs and rise awareness, so we created the cop. (Fixes #1748)

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.

@swelther swelther force-pushed the feature/do-not-use-remove-const branch from 6345386 to a3b8236 Compare December 1, 2023 15:08
@swelther swelther marked this pull request as ready for review December 1, 2023 15:09
pirj
pirj approved these changes Dec 2, 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.

Looks good already. Up to you if you want to enhance this to detect const removal at any level.
Thank you for the contribution!

lib/rubocop/cop/rspec/remove_const.rb Show resolved Hide resolved
lib/rubocop/cop/rspec/remove_const.rb Outdated Show resolved Hide resolved
@swelther swelther force-pushed the feature/do-not-use-remove-const branch 4 times, most recently from 4a26dc5 to 42dc2ab Compare December 4, 2023 08:48
@pirj pirj requested review from bquorning, Darhazer and ydah December 4, 2023 10:46
@ydah
Copy link
Member

ydah commented Dec 4, 2023

Could you run rake generate_cops_documentation and add docs/ to the commit?

@swelther
Copy link
Contributor Author

swelther commented Dec 4, 2023

oops, I thought I did this already, I'll have a look

@swelther swelther force-pushed the feature/do-not-use-remove-const branch from 42dc2ab to 775ad8f Compare December 4, 2023 13:09
@swelther
Copy link
Contributor Author

swelther commented Dec 4, 2023

ah, had to re-run the task because I adapted the documentation in the code. pushed :)

pirj
pirj reviewed Dec 4, 2023
View reviewed changes
lib/rubocop/cop/rspec/remove_const.rb
# before do
# SomeClass.send(:remove_const, :SomeConstant)
# end
#
Copy link
Member

@pirj pirj Dec 4, 2023 •
edited by bquorning
Loading

Choose a reason for hiding this comment

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

Does it make sense to add an exqmple with stub_const as good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question 🤔 We had some spec helper that used Object.remove_const and Object.const_set to change a global config in our specs. This caused several issues in our CI so I added this as an example. But I'm no longer sure if this is a good example in the wild?

bquorning
bquorning reviewed Dec 4, 2023
View reviewed changes
lib/rubocop/cop/rspec/remove_const.rb

# @!method remove_const(node)
def_node_matcher :remove_const, <<~PATTERN
(send _ {:send | :__send__} (sym :remove_const) _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

More a comment for @pirj than for @swelther:

I don’t know that the | is necessary, but I found that any of these 3 patterns will let the current specs pass:

  1. (send _ {:send | :__send__} (sym :remove_const) _)
  2. (send _ {:send :__send__} (sym :remove_const) _)
  3. (send _ {:send __send__} (sym :remove_const) _)

Curiously, (send _ {:send | __send__} (sym :remove_const) _) will not work.

Copy link
Member

Choose a reason for hiding this comment

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

Ha, another node pattern related gotcha gotcha in the same PR!

Agree, | is optional, but it actually helped finding a match-all node.
__send__ is a named “any node”, basically the same as _. https://docs.rubocop.org/rubocop-ast/node_pattern.html had an example with (int _value). I think we used that in our cops, too, but it’s not documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, do I need to change something? Sorry, I'm a bit lost here 😅

Copy link
Member

Choose a reason for hiding this comment

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

You may want to remove the | as it’s redundant, or to keep it for failsafe. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the | syntax more because it looks like regex and might be easier to understand. Would like to keep it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the docs I found this bit:

If all the branches have a single term, you can omit the |, so {int | float} can be simplified to {int float}.

So, apparently it’s valid syntax – I just hadn’t seen it before 😳 Let’s just leave the | in.

@swelther swelther force-pushed the feature/do-not-use-remove-const branch from 775ad8f to 754e75d Compare December 12, 2023 12:55
@bquorning bquorning merged commit fb85f88 into rubocop:master Dec 14, 2023
22 checks passed
LukasAud added a commit to puppetlabs/rspec-puppet that referenced this pull request Jan 9, 2024
@LukasAud
(MAINT) Disable new Rspec/RemoveConst cop
b246ecb 
Following the recent introduction of a new cop in Rubocop,
Rspec/RemoveConst, our CI has started failing. It seems like this rule
is being enforced as a good practice to avoid buggy debug information
but no real alternative is being offered.

This commit disables the cop to allow the CI to continue working. Here
are the discussions that led to the creation of the rule:
rubocop/rubocop-rspec#1748
rubocop/rubocop-rspec#1749
@LukasAud LukasAud mentioned this pull request Jan 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirj pirj pirj approved these changes

@bquorning bquorning bquorning approved these changes

@Darhazer Darhazer Awaiting requested review from Darhazer

@ydah ydah Awaiting requested review from ydah

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

Successfully merging this pull request may close these issues.

Cop idea: Don't use remove_const in specs
4 participants
@swelther @ydah @pirj @bquorning