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
Conversation
844fdae
to
6ef1044
Compare
There was a problem hiding this 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.
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. |
The noise is why i have Enabled: false. |
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
9927671
to
0790243
Compare
This is a separate commit so we can easily not do it if you'd rather not
Thanks! |
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 shouldbe Enabled by default.
I didn't bother creating an autocorrect because I see there being two
options and thought I'd get feedback.
missing a
::
prefix. This has the advantage of probably looking correctmore often, but also being wrong a lot.
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
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.