-
-
Notifications
You must be signed in to change notification settings - Fork 266
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 Rubocop::Cops::Rails::ZeitwerkFriendlyConstant
#1240
base: master
Are you sure you want to change the base?
Conversation
I've tried this one out and its flagging a bunch of things I'm not sure are actually problems.
It however also exposed a few cases where test classes contains typos/copy-paste issues, or where a class is nested wrongly which I think is the main purpose of this cop. |
Oh yeah, neglected to mention we have this in our config: Exclude:
- 'bin/**/*'
- 'test/**/*'
- 'lib/**/*'
- 'gems/**/*'
- 'config/**/*'
- '**/config/initializers/*'
- 'components/*/config/**/*'
- 'db/**/*'
- 'components/**/lib/**/*'
- 'components/**/test/**/*'
|
Yes, this indeed is the main purpose of the cop. I created it in the context of a project where we were running each of our tests from our CI suite in a local (lazy-loaded) environment, which exposed issues related to load order that were not apparent in an eager-loaded environment. But since then, it has also come in handy to avoid issues that crop up in cases where you have: # foo/bar.rb
module Foo
class Bar; end
class SomeOtherClass; end
end
# baz.rb
module Baz
# some code that references `Foo::Bar`
# some code that references `Foo::SomeOtherClass`
end In this context, if the line that references |
cc @fxn You might find this interesting 😄 |
With these, yes. It looks much better. It leaves me with one legit offense and one that's somewhat my fault because of unconventional repo structure. You will need to add something like this to the default config since you can't expect users do to this themselves, especially for a cop that would be enabled by default (or rather be pending). You're also excluding the test folder but at least for me that gave me some nice finds and no false positives. But I guess that depends on how you write/name them. They aren't part of autoloading (I think), so not that important. Considering the many ways you can structure your code (rspec instead of minitest is one I can immediatly think of), in my mind using an explicit includelist of I think most if not all autoloading for a traditional rails app is going on in the For the acronyms/inflections: If you use them heavily you will also need to modify the cops config the same way. I believe it would makes sense to at least ignore casing differences by default. So Anyways, I'm not a maintainer here, etc. etc. These are just my personal thoughts. You might be better off to wait for someone with actual authority unless some of my points make perfect sense to you. |
❤️ It is going to be useful to have a cop preventing this kind of situations. I wonder about the inherent limitations the cop may have, and whether they could break user expectations. For example, we know inflection rules can go beyond acronyms, there might be collapsed directories, custom namespaces, and ignored files or subtrees. Given a certain file path, the loader has API to tell you which is its expected constant path. That would be robust and simple, but I guess RuboCop can only work with source code and should not boot the app. |
Yep, we left those out since technically they are not autoloaded, so Zeitwerk is not involved. But that was just our preference.
Is it possible to set default include/exclude list for a cop? I didn't see anything like that, and honestly other than this one cop, I haven't worked that deeply with rubocop in general.
That would change the logic of the cop somewhat, and in our case, on a huge codebase, just setting a list of acronyms (mostly copied from
Yep, and all these things are reasons we haven't released this cop beyond our use in one repo thus far. But given that all those use cases are probably a small minority (I think?) I think the default setup here would be useful for a majority of cases (with config for acronyms, include/exclude etc) But it may also "nudge" folks to not stray from the "standard" of what is supported here, which may be a small negative I suppose.
Yes, this is the eternal issue of static vs runtime. If we could assume a booted app, then the acronyms config would also be unnecessary. |
Oh yeah, you can totally do this. Check out rubocop-rails/config/default.yml Lines 133 to 139 in aa30af9
Exclude works the same. |
9e7cf24
to
02524a9
Compare
@Earlopain thanks for the tip, I added a default include directive to only include ruby files in the |
02524a9
to
a8552f5
Compare
This PR adds a cop named
Rails/ZeitwerkFriendlyConstant
.The cop ensures that all constants defined in files are independently autoloadable by Zeitwerk. The inline comments in the cop explain this, but in a nutshell, this avoids situation like:
The way it checks this is by crawling the AST and tracking the nesting of constants relative to the file path nesting. The cop makes no assumptions about autoload paths, it simply looks for any possible inconsistencies between the constants defined and the filename nesting.
We use this at Shopify and it has helped weeding out issues that can arise when load paths change and suddenly previously "loadable" constants suddenly break, as in the example above.
To use the cop, you'll need to ensure you exclude file paths that are not autoloaded. There is also an
Acronyms
config to define acronyms, roughly matching what you might define withActiveSupport::Inflector.inflections
.Here's our config for reference (
components
has all our in-repo engines):Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.md
if the new code introduces user-observable changes. See changelog entry format for details.