-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Include **/factories.rb
in the inspection target
#104
base: master
Are you sure you want to change the base?
Conversation
@@ -2,9 +2,8 @@ | |||
FactoryBot: | |||
Enabled: true | |||
Include: | |||
- "**/spec/factories.rb" | |||
- "**/factories.rb" |
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.
I’m uncertain if it’s deliberate or not, but they would also load from factories/**/*.rb
Yet another doc https://github.com/thoughtbot/factory_bot/blob/cf3f21fcbbccbc40849c74a84eeb0bb0bd6c7606/docs/src/defining/file-paths.md?plain=1#L7 yhat doesn’t mention this consistently.
@@ -2,9 +2,8 @@ | |||
FactoryBot: | |||
Enabled: true | |||
Include: | |||
- "**/spec/factories.rb" | |||
- "**/factories.rb" |
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.
But would we also analyze eg app/presenters/factories.rb?
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.
Yes, but I think this is a risk we should accept when we decide to put **/
at the beginning of Include
. It is also inconsistent that other Include
s support it, but not only here.
Since it is rare for a constant to be defined in the plural form, I believe that this is rarely a problem in practice.
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.
I understand that factory_bot defaults to **/factories.rb as the definition file path. However, I doubt that it should be the default for rubocop-factory_bot. Because I am concerned about the increased risk of false positives, as also #104 (comment).
Also, even if some users wanted to fix this, I felt it was not necessary to include it in the default because it could be avoided by simply adding the target path to FactoryBot: Include:
.
According to the factory_bot documentation, the default
FactoryBot.definition_file_paths
also includesfactories.rb
.Therefore, it would be reasonable to include this file in the inspection target.
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.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
.Enabled: pending
inconfig/default.yml
.VersionAdded: "<<next>>"
indefault/config.yml
.If you have modified an existing cop's configuration options:
VersionChanged: "<<next>>"
inconfig/default.yml
.