rubocop-rspec
rubocop-rspec copied to clipboard
LeakyConstantDeclaration - allow definitions on example group
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
inconfig/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>>"
indefault/config.yml
.
If you have modified an existing cop's configuration options:
- [ ] Set
VersionChanged: "<<next>>"
inconfig/default.yml
.
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?
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?
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.
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 let
s.
Allowing any explicitly specified namespace would make sense to me - even ::. If you do that intentionally, the cop should respect that.
Agree.