Add assocaitions_exclude to exclude files from association inspection #380
Conversation
gmcgibbon
force-pushed
the
support_non_ar_association
branch
from
1c65b1b
to
6723041
Compare
|
Could it be an option to allow users to define a path glob for ignoring associations in? ...I'm not sure which solution I'd prefer... but I am also not sure that it makes sense to take on this complexity in packwerk itself. Maybe the whole association handling could be moved into a plugin, which could then also handle serializers? |
|
|
||
| sig { params(node: AST::Node).returns(T.nilable(Symbol)) } | ||
| def type(node) | ||
| case NodeHelpers.source_location(node).match(%r{\bapp/(.*)/}).to_a.last |
There was a problem hiding this comment.
Checking the folder of the file is too brittle. Active Record subclasses can be anywhere, for example in subdirectories of app/models
There was a problem hiding this comment.
If we don't filter paths somehow the bug still exists for AMS users. Maybe we can pull ignore paths from a configuration file if we think checking for app/models is too brittle?
There was a problem hiding this comment.
Can we check the context where has_many is called? This is tricky because you could make a case that serializers can also be inside app/models.
If we can't check the context, I'm ok with a exclude patter, so people that use AMS can say to not check on app/serializers
There was a problem hiding this comment.
So from my understanding, we can only check what is statically printed in the file. There's no way to guarantee a superclass if a model without actually running the code or making unsafe assumptions.
Checking the superclass of the context is ActiveRecord::Base or ApplicationRecord is too brittle (because it doesn't account for custom record subclasses), and serializers have the same problem with their immediate subclass. We could check if the superclass or class end with Record, but that is an unsafe assumption for non-abstract models. We could try to resolve superclasses that aren't Active Record default, but if any of them aren't autoloaded with Zeitwerk, we won't be able to see them.
I think an exclude glob option covers all of our bases and avoids taking on this complexity.
There was a problem hiding this comment.
Updated the PR to take this approach.
gmcgibbon
force-pushed
the
support_non_ar_association
branch
from
6723041
to
4956ae8
Compare
gmcgibbon
force-pushed
the
support_non_ar_association
branch
4 times, most recently
from
8d8a79e
to
2f19cbe
Compare
gmcgibbon
changed the title
Other libraries (like Active Model Serializers) use has_one / has_many macros like Active Record does. We don't need to pay attention to this. Instead of guessing what a node is based on what we can see in the file, we are using an exclude glob because it is too hard to know if a class is an Active Record or not without guessing.
gmcgibbon
force-pushed
the
support_non_ar_association
branch
from
2f19cbe
to
0923c43
Compare
|
@rafaelfranca does this approach align with your request for changes? |
|
@gmcgibbon @rafaelfranca is there any reason why we can't merge this PR? |
|
@zumkorn no, thanks for the reminder. I'll merge now! @rafaelfranca if you have any additional feedback let's fix forward. |
What are you trying to accomplish?
Fixes #370
What approach did you choose and why?
Other gems like ActiveModel::Serializers define association methods that reference serializers. We can ignore these files entirely with an exclude pattern.
What should reviewers focus on?
The exclude pattern breaks public API, and we only get guarantees from Sorbet if the plugin package uses it (and they don't have to). So, I added an ArgumentError check to make sure all we still support the old signatures without
relative_file. We could alternatively proceed without this precaution, so I can drop the commit if needed.Type of Change
Additional Release Notes
Include any notes here to include in the release description. For example, if you selected "breaking change" above, leave notes on how users can transition to this version.
If no additional notes are necessary, delete this section or leave it unchanged.
Checklist