-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix a false positive for FactoryBot/AssociationStyle
when EnforcedStyle: Explicit
and using trait within trait
#59
Conversation
node.each_ancestor(:block).any? do |ancestor| | ||
if ancestor.send_node.method?(:factory) | ||
trait_name(ancestor).include?(node.method_name) | ||
end | ||
end |
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.
Performance wise, shouldn't we compile a list of traits on the very definition of the factory, and then reuse it. Or just go to the top block, as now you're traveling part of the tree every time you are going up (when you want to look also into the siblings)
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 updated this PR. Thank you.
@@ -158,7 +163,8 @@ def autocorrect_to_implicit_style(corrector, node) | |||
|
|||
def bad?(node) | |||
if style == :explicit | |||
implicit_association?(node) | |||
implicit_association?(node) && |
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.
Cosmetic. Here it feels that ‘implicit_association?’ can return true when it only looks like an implicit association, not necessarily is.
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.
Maybe we should rename the matcher? I can't think of a good name now 🤔
…Style: Explicit` and using trait within trait Fix: #54
I updated this PR. |
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.
Looks good.
Have you had a chance to run it against a real codebase?
I ran
|
Fix: #54
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).