https://pantsbuild.org/ logo
Powered by
b

bitter-ability-32190

08/26/2022, 12:10 AM
A recent change made union subclasses also have the members of all parent union classes, but I don't think we really discussed it and I'm kinda thinking we might want to look at it from a step back. There's about to be use cases for needing it either direction, so I'm wondering which direction is the most "natural". Should a union subclass inherit the parent members? or should a parent inherit all the subclass members? Example in 🧵
Consider:
Copy code
@union
    class Fruit:
        pass

    class Banana(Fruit):
        pass

    class Apple(Fruit):
        pass

    @union
    class CitrusFruit(Fruit):
        pass

    class Orange(CitrusFruit):
        pass
along with:
Copy code
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.
CC @witty-crayon-22786 @hundreds-father-404 @curved-television-6568
There's also a third option I could also see us doing: Don't do either, but allow the user to request this info:
Copy code
union_membership.get(CitrusFruit, include_ancestor_members=True)
union_membership.get(CitrusFruit, include_descendants_members=True)
👍 1
h

hundreds-father-404

08/26/2022, 12:14 AM
I think I probably lean towards the third. Or reverting the original PR and going back to needing to explicitly specify everything, tbh
c

curved-television-6568

08/26/2022, 12:37 AM
Or reverting the original PR and going back to needing to explicitly specify everything, tbh
There was a motivating use case that introduced this in the first place. I’ll provide an example:
Copy code
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.
h

hundreds-father-404

08/26/2022, 12:39 AM
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.
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 orgs
c

curved-television-6568

08/26/2022, 12:40 AM
Would it be that asking for members of 
CitrusFruit
should give 
{Orange, Banana, Apple}
, or that asking for 
Fruit
gives 
{Orange, Banana, Apple}
?
I would say that, per the union rules provided, that members for 
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 orgs
Indeed. That is why it is in a backend that you opt in to.
👍 1
we can have a test in pantsbuild/pants that every registered target has the field registered
I’m not sure I follow this one..
b

bitter-ability-32190

08/26/2022, 12:42 AM
Oh I had no idea that was the motivation. To inject into every target 🤔
I would say that, per the union rules provided, that members for 
CitrusFruit
is just 
Orange
and members for 
Fruit
is 
Banana
, 
Apple
(and 
Orange
I guess, not sure about the last one here..?)
So FWIW that doesn't map with todays code 😛
🙈 1
c

curved-television-6568

08/26/2022, 12:42 AM
I think it was described in the introducing PR…
b

bitter-ability-32190

08/26/2022, 12:43 AM
c

curved-television-6568

08/26/2022, 12:44 AM
I was clearly not considering union membership as much as getting my desired end result out of it 🙊
h

hundreds-father-404

08/26/2022, 12:44 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 forget
c

curved-television-6568

08/26/2022, 12:44 AM
I’ll pair up with the fix with you on this one if needed Josh
h

hundreds-father-404

08/26/2022, 12:44 AM
Generally, I'd rather have some annoyance with us developing than add inheritance semantics to UnionMembership that are tricky to understand
c

curved-television-6568

08/26/2022, 12:46 AM
I agree with fixing the trickiness out of the union behaviour, but would prefer to do that without loosing the ability to register a plugin field and have it propagate through to all subclassed target types. (it would be surprising to me if a subclassed target doesn’t inherit the registered plugin fields)
b

bitter-ability-32190

08/26/2022, 12:47 AM
I think there's a more targetd way of fixing that
pun intended 🙂
I'll try it out now
👍 2
c

curved-television-6568

08/26/2022, 12:47 AM
haha, I’m in favour of doing that, it’s all about the end result for me 🙂
❤️ 1
> 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 forget
Well, 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)
h

hundreds-father-404

08/26/2022, 12:50 AM
It will make sure pantsbuild/pants is good to go. You're right it won't help plugin authors
c

curved-television-6568

08/26/2022, 12:51 AM
I think, for the visibility feature, it’s a non-starter to offer something that only works for core pants and not generically for third party plugins. 😉
b

bitter-ability-32190

08/26/2022, 12:55 AM
Yeah OK fixed all this mess, lol
❤️ 2
PR incoming
c

curved-television-6568

08/26/2022, 1:17 AM
I owe you at least 2️⃣ 🍻 for that one Josh. Good refactor. 🙌
❤️ 2
b

bitter-ability-32190

08/26/2022, 1:17 AM
❤️
Powered by