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.

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

LeakyConstantDeclaration - allow definitions on example group #1798

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

naveg
Copy link
Contributor

@naveg naveg commented Feb 5, 2024

Constant, classes, and modules that are defined on the example group do
not leak into the global namespace. While doing this may not be
a particularly good practice, it should ideally not result in
an offense.

The practice is described here: https://makandracards.com/makandra/47189-rspec-how-to-define-classes-for-specs#section-1-defining-the-constant-on-the-example-class


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.

@naveg naveg force-pushed the define-on-example-group branch from 72f563f to 51f9e38 Compare February 5, 2024 23:16
Constant, classes, and modules that are defined on the example group do
not leak into the global namespace. While doing this may not be
a particularly good practice, it should ideally not result in
an offense.

The practice is described here: https://makandracards.com/makandra/47189-rspec-how-to-define-classes-for-specs#section-1-defining-the-constant-on-the-example-class
@naveg naveg force-pushed the define-on-example-group branch from 51f9e38 to b11854b Compare February 5, 2024 23:19
@naveg naveg marked this pull request as ready for review February 5, 2024 23:21
@naveg naveg requested a review from a team as a code owner February 5, 2024 23:21
@pirj
Copy link
Member

pirj commented Feb 6, 2024

Sorry, I didn’t read the article, but the description of this is misleading.
With self:: (or anything:: really), you explicitly specify the owner of the constant.
The problem that this cop solves is auto-detection of the owner that defaults to the top-level namespace, which is unintuitive and confusing.

I’m fine to accept this if we change the scope to ignore all const definitions with explicitly specified owner.
Or is it problematic for examples?

@pirj
Copy link
Member

pirj commented Feb 6, 2024

The problem with the approach described in the article is that examples may litter class, and those changes would persist between examples.

The cop was initially introduced to avoid side effects and surprises. I’d love it to keep what it’s doing. https://fili.pp.ru/leaky-constants.html

I’m more inclined to close the PR. WDYT @rubocop/rubocop-rspec?

@naveg
Copy link
Contributor Author

naveg commented Feb 6, 2024

The problem with the approach described in the article is that examples may litter class, and those changes would persist between examples.

This is true of any class defined in any location. The issue flagged by the cop is that it's quite easy to define classes in the global namespace without realizing that is happening. Using self:: (or any other namespace, as you point out) resolves that.

Allowing any explicitly specified namespace would make sense to me - even ::. If you do that intentionally, the cop should respect that.

@pirj
Copy link
Member

pirj commented Feb 6, 2024

This is true of any class defined in any location.

let(:klass) { Class.new { } } won’t suffer from this.
But in general - yes, it’s possible to use regular variables in example groups like klass = “hello”, and it would be possible to alter it, too. This is why we have lets.

Allowing any explicitly specified namespace would make sense to me - even ::. If you do that intentionally, the cop should respect that.

Agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants