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

Add Lint/ConstantResolution cop #8142

Merged
merged 3 commits into from Jun 20, 2020
Merged

Add Lint/ConstantResolution cop #8142

merged 3 commits into from Jun 20, 2020

Conversation

robotdana
Copy link
Contributor

@robotdana robotdana commented Jun 11, 2020

Ruby constant resolution sometimes gets things wrong.

I think generally gems (that are meant for including as a library, not
like rubocop itself) should fully qualify all constants to avoid
stepping on any toes inadvertently. I would run this on my gems without
using Only/Ignore

Large projects will probably end up with one or two constant names that
are problematic because of a conflict with a library or just internally
having sensible names for directories that end up being used by e.g.
rails as namespaces in multiple different places. I would run these with
Only: [The, Constant, Names, Causing Issues]

Sometimes a method refers to a constant that you want to refer to a
different constant at different calls, e.g. the method is defined on a
superclass and the constant differs between subclasses. (e.g. MSG in
rubocop add_offense). I would use
Ignore: [The, Intentionally, Abiguous, Constant] in these cases.
(or self.class::MSG now that I've looked up how add_offense works)

I've left this Enabled: false because I'm 50-50 on whether it should
be Enabled by default.

I didn't bother creating an autocorrect because I see there being two
options and thought I'd get feedback.

  1. carelessly assume the written constant is fully qualified and just
    missing a :: prefix. This has the advantage of probably looking correct
    more often, but also being wrong a lot.
  2. look at the current namespace and prefix that. This has the advantage
    of not necessarily changing behaviour for classes. For modules leaving
    it unchanged or carelessly assuming, or checking if the constant is
    defined in the module or not before prefixing
  3. just don't autocorrect
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@robotdana robotdana force-pushed the constants branch 2 times, most recently from 844fdae to 6ef1044 Compare June 12, 2020 00:02
Copy link
Collaborator

@Drenmi Drenmi left a comment

Choose a reason for hiding this comment

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

Code looks great to me. 🚀

My only wonder is if this cop doesn't deserve an upgrade to the Lint namespace, since it deals with code that can lead to unexpected behaviour.

lib/rubocop/cop/style/constant_resolution.rb Outdated Show resolved Hide resolved
@robotdana robotdana changed the title Add Style/ConstantResolution cop Add Lint/ConstantResolution cop Jun 12, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 12, 2020

Btw, won't this cause a lot noise for references to "naked" constants in namespaced modules/classes? I like the general idea, but we have to careful not to generate a ton of offenses with the new cop. I'm wondering how we can improve the situation, though. That's a tough nut to crack on a single-file basis.

@robotdana
Copy link
Contributor Author

The noise is why i have Enabled: false.

@bbatsov
Copy link
Collaborator

bbatsov commented Jun 12, 2020

Ah, sorry. I totally missed that!

Now I'm wondering if that's truly a lint check instead a style check, as lint checks are typically enabled by default and they typically target more serious problems in the code. Anyways, let's keep it under Lint for now. I see you wrote some great explanations in the description of the PR (which I should have read more carefully) and I feel it's a good idea to move a lot of this to the rationale of the cop, so it's easier for people to understand how to use it and why it's disabled by default.

Ruby constant resolution sometimes gets things wrong.

I think generally gems (that are meant for including as a library, not
like rubocop itself) should fully qualify all constants to avoid
stepping on any toes inadvertently. I would run this on my gems without
using `Only`/`Ignore`

Large projects will probably end up with one or two constant names that
are problematic because of a conflict with a library or just internally
having sensible names for directories that end up being used by e.g.
rails as namespaces in multiple different places. I would run these with
`Only: [The, Constant, Names, Causing Issues]`

Sometimes a method refers to a constant that you want to refer to a
different constant at different calls, e.g. the method is defined on a
superclass and the constant differs between subclasses. (e.g. MSG in
rubocop add_offense). I would use
`Ignore: [The, Intentionally, Abiguous, Constant]` in these cases.
(or `self.class::MSG` now that I've looked up how add_offense works)

I've left this `Enabled: false` because I'm 50-50 on whether it should
be Enabled by default.

I didn't bother creating an autocorrect because I see there being two
options and thought I'd get feedback.

1. carelessly assume the written constant is fully qualified and just
missing a ::` prefix. This has the advantage of probably looking correct
more often, but also being wrong a lot.
2. look at the current namespace and prefix that. This has the advantage
of not necessarily changing behaviour for classes. For modules leaving
it unchanged or carelessly assuming, or checking if the constant is
defined in the module or not before prefixing
3. just don't autocorrect
@robotdana robotdana force-pushed the constants branch 2 times, most recently from 9927671 to 0790243 Compare June 20, 2020 03:22
This is a separate commit so we can easily not do it if you'd rather not
@bbatsov bbatsov merged commit 3c5838a into rubocop:master Jun 20, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Jun 20, 2020

Thanks!

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.

None yet

3 participants