Plugin fields should propagate to subclassed target types. #15876
Conversation
Consider subclassed `PythonSourceTarget`. Now, all plugin fields that are registered on `PythonSourceTarget` should also be registered on the custom subclassed target, otherwise subclassing a target will lose any plugin fields in doing so, making it very difficult to customize targets without inadvertently losing functionality added by plugin fields. This is addressed by ensuring that the Target `PluginField` class mirrors the class hierarchy of the owning Target class, and when registering unions all subclassed union bases are registered with the same union members as the subclassed union base (in addition to any directly registered for the subclassed union base type). [ci skip-rust]
|
From the design doc comment thread: https://docs.google.com/document/d/1DdVZ4DqX7uwiYuCL_ntb17yPxyez1fbiu8K0whZ78Lg/edit#
In other words, this is prep-work to support adding the Visibility feature entirely as a plugin. |
There was a problem hiding this comment.
@Eric-Arellano definitely better understands this code: will let them take a look.
| @@ -72,6 +72,14 @@ def from_rules(cls, rules: Iterable[UnionRule]) -> UnionMembership: | |||
| mapping: DefaultDict[type, OrderedSet[type]] = defaultdict(OrderedSet) | |||
| for rule in rules: | |||
| mapping[rule.union_base].add(rule.union_member) | |||
| # Subclassed union bases should inherit the superclass's union members. | |||
There was a problem hiding this comment.
This seems like a separate change?
There was a problem hiding this comment.
From the main change with plugin fields being inherited. I don't see why unions are involved?
There was a problem hiding this comment.
Ah.
Otherwise union.get(SomeTarget.PluginFields) wouldn't give you anything when you register a plugin field on Target.
|
It's for opening up for the visibility feature to be in a backend so it's opt-in. With this change we can register the visibility field as a plugin field on the Target base class so all targets get it without having to explicitly register it on all individually. |
|
Did you see the comment thread from the visibility design doc in a previous comment in this PR. Thought it explained the motivation for this pretty well.? |
|
Also, I think it removes a surprising gotcha. If I subclass the PythonDistribution target class for instance, in order to customize it for some reason, I no longer have any of the plugin fields for it. |
Before: ``` >>> print(union_membership.get(BlackRequest)) FrozenOrderedSet([<class 'pants.backend.python.lint.autoflake.rules.AutoflakeRequest'>, <class 'pants.backend.python.lint.black.rules.BlackRequest'>, <class 'pants.backend.python.lint.docformatter.rules.DocformatterRequest'>, <class 'pants.backend.python.lint.isort.rules.IsortRequest'>, <class 'pants.backend.shell.lint.shfmt.rules.ShfmtRequest'>, <class 'pants.backend.go.lint.gofmt.rules.GofmtRequest'>, <class 'pants.backend.java.lint.google_java_format.rules.GoogleJavaFormatRequest'>, <class 'pants.backend.scala.lint.scalafmt.rules.ScalafmtRequest'>]) ``` After: ``` >>> print(union_membership.get(BlackRequest)) FrozenOrderedSet([]) ``` This was introduced in #15876 which was first in 2.14.
…build#16645) Before: ``` >>> print(union_membership.get(BlackRequest)) FrozenOrderedSet([<class 'pants.backend.python.lint.autoflake.rules.AutoflakeRequest'>, <class 'pants.backend.python.lint.black.rules.BlackRequest'>, <class 'pants.backend.python.lint.docformatter.rules.DocformatterRequest'>, <class 'pants.backend.python.lint.isort.rules.IsortRequest'>, <class 'pants.backend.shell.lint.shfmt.rules.ShfmtRequest'>, <class 'pants.backend.go.lint.gofmt.rules.GofmtRequest'>, <class 'pants.backend.java.lint.google_java_format.rules.GoogleJavaFormatRequest'>, <class 'pants.backend.scala.lint.scalafmt.rules.ScalafmtRequest'>]) ``` After: ``` >>> print(union_membership.get(BlackRequest)) FrozenOrderedSet([]) ``` This was introduced in pantsbuild#15876 which was first in 2.14. # 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]
…build#16645) Before: ``` >>> print(union_membership.get(BlackRequest)) FrozenOrderedSet([<class 'pants.backend.python.lint.autoflake.rules.AutoflakeRequest'>, <class 'pants.backend.python.lint.black.rules.BlackRequest'>, <class 'pants.backend.python.lint.docformatter.rules.DocformatterRequest'>, <class 'pants.backend.python.lint.isort.rules.IsortRequest'>, <class 'pants.backend.shell.lint.shfmt.rules.ShfmtRequest'>, <class 'pants.backend.go.lint.gofmt.rules.GofmtRequest'>, <class 'pants.backend.java.lint.google_java_format.rules.GoogleJavaFormatRequest'>, <class 'pants.backend.scala.lint.scalafmt.rules.ScalafmtRequest'>]) ``` After: ``` >>> print(union_membership.get(BlackRequest)) FrozenOrderedSet([]) ``` This was introduced in pantsbuild#15876 which was first in 2.14.
Consider subclassed
PythonSourceTarget. Now, all plugin fields that are registered onPythonSourceTargetshould also be registered on the custom subclassed target, otherwisesubclassing a target will lose any plugin fields in doing so, making it very difficult to customize
targets without inadvertently losing functionality added by plugin fields.
This is addressed by ensuring that the Target
PluginFieldclass mirrors the class hierarchy of theowning Target class, and when registering unions all subclassed union bases are registered with the
same union members as the subclassed union base (in addition to any directly registered for the
subclassed union base type).
[ci skip-rust]