bitter-ability-32190
08/26/2022, 12:10 AM@union
class Fruit:
pass
class Banana(Fruit):
pass
class Apple(Fruit):
pass
@union
class CitrusFruit(Fruit):
pass
class Orange(CitrusFruit):
pass
along with:
UnionRule(Fruit, Banana),
UnionRule(Fruit, Apple),
UnionRule(CitrusFruit, Orange),
Would it be that asking for members of CitrusFruit
should give {Orange, Banana, Apple}
, or that asking for Fruit
gives {Orange, Banana, Apple}
?
My own gut says the latter.union_membership.get(CitrusFruit, include_ancestor_members=True)
union_membership.get(CitrusFruit, include_descendants_members=True)
hundreds-father-404
08/26/2022, 12:14 AMcurved-television-6568
08/26/2022, 12:37 AMOr reverting the original PR and going back to needing to explicitly specify everything, tbhThere was a motivating use case that introduced this in the first place. I’ll provide an example:
class MyTarget (Target):
...
def rules():
return [MyTarget.register_plugin_field(SomePluginField)]
# ---
class OthersPluginFromSomewhereElse (MyTarget):
...
Now, the users of OthersPluginFromSomewhereElse
targets would expect to find a SomePluginField
on their targets, as they’re being subclassed from MyTarget
, however it would be a pain to require the author of OthersPluginFromSomewhereElse
to redeclare all plugin fields registered on MyTarget
explicitly, as that may be unknown/change over time.
The use case for this in particular is for the visibility feature, in order to support it as a backend that you can optionally enable, it hooks into the system by registering a new plugin field on the base Target
class in order to have it propagated to all and every target type there is. (which actually differs slightly from the example above, but hey..)
Without this ability, it would require the visibility backend to loop over all targets and register its field on each one of them, but for that to work, it would have to be loaded dead last of all plugins or it would risk to not register on some targets.hundreds-father-404
08/26/2022, 12:39 AMNow, the users of OthersPluginFromSomewhereElse targets would expect to find a SomePluginField on their targets, as they’re being subclassed from MyTarget, however it would be a pain to require the author of OthersPluginFromSomewhereElse to redeclare all plugin fields registered on MyTarget explicitly, as that may be unknown/change over time.It's a pain, but maybe fine? And we can have a test in pantsbuild/pants that every registered target has the field registered Note that the current behavior forces a user to have the
visibility
field from pantsbuild/pants, but they may want their own field instead. We've heard from users they want to do visibility different than us, and I think it's known we can't satisfy everyone. The design is meant to be good for most orgscurved-television-6568
08/26/2022, 12:40 AMWould it be that asking for members ofI would say that, per the union rules provided, that members forshould giveCitrusFruit
, or that asking for{Orange, Banana, Apple}
givesFruit
?{Orange, Banana, Apple}
CitrusFruit
is just Orange
and members for Fruit
is Banana
, Apple
(and Orange
I guess, not sure about the last one here..?)The design is meant to be good for most orgsIndeed. That is why it is in a backend that you opt in to.
we can have a test in pantsbuild/pants that every registered target has the field registeredI’m not sure I follow this one..
bitter-ability-32190
08/26/2022, 12:42 AMI would say that, per the union rules provided, that members forSo FWIW that doesn't map with todays code 😛is justCitrusFruit
and members forOrange
isFruit
,Banana
(andApple
I guess, not sure about the last one here..?)Orange
curved-television-6568
08/26/2022, 12:42 AMbitter-ability-32190
08/26/2022, 12:43 AMcurved-television-6568
08/26/2022, 12:44 AMhundreds-father-404
08/26/2022, 12:44 AMI’m not sure I follow this one..Like, we can add a test to make sure that us pantsbuild/pants developers don't forget to register the union rule. It's a pain to still add, but at least we know we won't forget
curved-television-6568
08/26/2022, 12:44 AMhundreds-father-404
08/26/2022, 12:44 AMcurved-television-6568
08/26/2022, 12:46 AMbitter-ability-32190
08/26/2022, 12:47 AMcurved-television-6568
08/26/2022, 12:47 AM> I’m not sure I follow this one..
Like, we can add a test to make sure that us pantsbuild/pants developers don’t forget to register the union rule. It’s a pain to still add, but at least we know we won’t forgetWell, this won’t work, as we can’t know up front which other plugins will be loaded with targets that needs the field registered on them, so would have to be done dynamically (and then it wouldn’t be a pain, as such I think)
hundreds-father-404
08/26/2022, 12:50 AMcurved-television-6568
08/26/2022, 12:51 AMbitter-ability-32190
08/26/2022, 12:55 AMcurved-television-6568
08/26/2022, 1:17 AMbitter-ability-32190
08/26/2022, 1:17 AM