Introduce FactoryGirl/FactoryClassName cop #839
Conversation
fragoulis
force-pushed
the
factorybot/factory-class-name
branch
7 times, most recently
from
b5ad34f to
bbd8652
Compare
There was a problem hiding this comment.
What is the reasoning for this cop?
I could only find this:
If the constant is not available (if you are using a Rails engine that waits to load models, for example), you can also pass a symbol or string, which factory_bot will constantize later, once you start building objects
|
I actually see an additional benefit in this style. Since factories usually cover all models, all models will be auto-loaded when |
The reason behind this as mentioned in the description is The loading overhead argument you mentioned is part of my argument as well, not explicitly mentioned though. |
|
Thanks for the comments. I believe this is ready. |
There was a problem hiding this comment.
Looks flawless!
Thanks for the contribution ❤️
Please squash the commits.
@Darhazer @bquorning please take a look.
|
|
||
| context 'when passing block' do | ||
| context 'when a class' do | ||
| it 'registers an offense' do |
There was a problem hiding this comment.
There's no guideline and even no common practice, but "ignores"/"flags" seems to be a little bit more widespread.
If you don't feel it fits the current spec structure that much, feel free to keep as is, it's good enough already.
There was a problem hiding this comment.
I guess you are referring to the use of the wording registers with the context above it.
Most of what I did I saw in other cops (factory bot ones). However, I can see there are 2 or 3 ways of doing things. It is normal for standards and common practices to change over time. It would be nice if the documentation was updated to reflect that, the best practices etc.
But anyway, it was a nice experience altogether. It will help me shape new cops for lots of things.
PS: I used the flags/ignores paradigm, I hope it is ok.
fragoulis
force-pushed
the
factorybot/factory-class-name
branch
3 times, most recently
from
ab5d473 to
c5b6552
Compare
|
@Darhazer, any comments? |
CHANGELOG.md
Outdated
| @@ -403,6 +403,7 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features. | |||
| * Add `RSpecMultipleDescribes` cop to check for multiple top level describes. ([@geniou][]) | |||
| * Add `RSpecDescribedClass` to promote the use of `described_class`. ([@geniou][]) | |||
| * Add `RSpecInstanceVariable` cop to check for the usage of instance variables. ([@geniou][]) | |||
| * Add `FactoryBot/FactoryClassName` cop. ([@jfragoulis][]) | |||
There was a problem hiding this comment.
Ah, just one more thing. Could you move this line up towards the top of the file, as the last bullet point under ## Master (Unreleased) please.
There was a problem hiding this comment.
…you‘ll need to rebase on latest master too, to avoid merge conflict.
There was a problem hiding this comment.
Wow, what an oversight! :) :) Done and thanks!
The cop ensures that when creating factories, we do not allow passing the class itself. Instead we require users to pass the class name. Although `FactoryBot` allows passing the class, we find that by doing so, application models are loaded during application initialization and many files are required without them having to. This affects the startup time as well as libraries like simplecov that fail to capture those files. Example: bad ``` factory :foo, class: Foo do end ``` good ``` factory :foo, class: 'Foo' do end ```
fragoulis
force-pushed
the
factorybot/factory-class-name
branch
from
c5b6552 to
da35c8d
Compare
|
Thanks and looking forward to your future contributions! 👍 |
The cop ensures that when creating factories and passing the class
name explicitly, we do not pass the class itself.
Although FactoryBot allows passing the class, we find that by doing so,
application models are loaded before the application itself gets loaded
(during factory definition).
This affects libraries like simplecov in that they fail to capture those files.
Example:
Checklist
master(if not - rebase it).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:
config/default.yml.