rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

LeakyConstantDeclaration - allow definitions on example group

Open naveg opened this issue 1 year ago • 4 comments

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:

  • [x] Feature branch is up-to-date with master (if not - rebase it).
  • [x] Squashed related commits together.
  • [x] Added tests.
  • [ ] Updated documentation.
  • [x] Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • [x] 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 avatar Feb 05 '24 23:02 naveg

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 avatar Feb 06 '24 05:02 pirj

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?

pirj avatar Feb 06 '24 05:02 pirj

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.

naveg avatar Feb 06 '24 07:02 naveg

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.

pirj avatar Feb 06 '24 15:02 pirj