Fix PluginField not working in production
#10469
Conversation
[ci skip-rust-tests]
src/python/pants/engine/target.py
Outdated
| # type so that registering a PluginField for one target doesn't end up registering for all | ||
| # targets. This means that we cannot use a statically declared class, so we instead | ||
| # dynamically create a class. We use a class property and name the method in PascalCase so | ||
| # that this implementation doesn't leak to call sites. |
There was a problem hiding this comment.
I'd prefer just a @memoized_classmethod and not pretending this is a nested type. Saying SomeTarget.plugin_field() is ~no more typing and less magic. Maybe better SomeTarget.register_plugin_field(Field) -> UnionRule - absolutely no magic.
There was a problem hiding this comment.
SomeTarget.register_plugin_field(Field) -> UnionRule I like that.
There was a problem hiding this comment.
I'm fine either way. It would be best if that ended up being a pattern that we could use elsewhere as well though... ie, Subsystem.register() -> TaskRule or something.
[ci skip-rust-tests]
There was a problem hiding this comment.
PTAL. I'm not sure this is much of an improvement - certainly makes unit tests more awkward.
There's also an argument that it's helpful to be transparent that this is using UnionRule. We use UnionRule for other plugin tasks, like adding a new linter, so it's consistent to have the user explicitly do that here.
src/python/pants/engine/target.py
Outdated
| # that this implementation doesn't leak to call sites. | ||
| def _anonymous_plugin_field_cls(cls) -> Type: | ||
| # NB: We make this a method, rather than using a static class, to ensure that every | ||
| # subclass has a distinct `.PluginField` class. This ensures that registering a plugin |
There was a problem hiding this comment.
The comment has drifted. There is no longer method vs static class nor .PluginField. Perhaps just focus on the 1 union per Target subtype need:
@final
@memoized_classproperty
def _plugin_field_union(cls) -> Type:
# N.B.: We want each Target subtype to collect its own union of plugin fields to ensure plugin
# fields don't leak across target types.
return union(type(f"{cls.__name__}PluginField", (), {}))
| @@ -262,7 +262,7 @@ class CustomField(BoolField): | |||
| alias = "custom_field" | |||
| default = False | |||
|
|
|||
| union_membership = UnionMembership({FortranTarget.PluginField: [CustomField]}) | |||
| union_membership = UnionMembership({FortranTarget._anonymous_plugin_field_cls: [CustomField]}) | |||
There was a problem hiding this comment.
This would be less awkward if UnionMembership had UnionMembership.from_rules(Iterable[UnionRule]) -> UnionMembership which it arguably should have already anyhow.
There was a problem hiding this comment.
Good call. I'll do that as a precursor PR.
True currently. I've had my long standing unpublished PR to eliminate UnionRule though. Plugin tasks need to today but need not in principle register anything if they are already subclassing an @union'd type. Assuming a future where we kill UnionRule, the return type of this registration method - and all registration methods we may add to form a pattern, would be the more opaque |
…ields [ci skip-rust-tests]
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
…les-api-file-system [ci skip-rust] [ci skip-build-wheels]
Eric-Arellano
force-pushed
the
fix-union-fields
branch
from
3a21c5e
to
328dd46
Compare
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Two issues:
UnionMembershipto theTargetconstructor, soPluginFields were being ignored.PluginFieldclass meant that registering a field on one target would register it on all targets.[ci skip-rust-tests]