pyblish / pyblish-base

Pyblish base library - see https://github.com/pyblish/pyblish for details.
Other
127 stars 59 forks source link

make Instance able to return all families, for convenience #331

Open davidlatwe opened 6 years ago

davidlatwe commented 6 years ago

I think this could provide a bit more convenience.

Instead of querying family and families from instance.data and adding them up every time we need that info, making that as a built-in would be nice, right ?

Happy new year :)

davidlatwe commented 6 years ago

commit 60f817b, I was referencing from this stackoverflow answer

mottosso commented 6 years ago

Thanks for this @davidlatwe, my only concern is that families now appear different than regular data, and that it won't be as obvious that it can change, and be manipulated as regular data.

You're right, that currently because we are in a transition period of having both family and families, reading it is a little awkward..

What do you think about instead overriding the call to .data["families"] such that it always returns all families, including family? Then, once we're done transitioning, there'll be no need for family and it can be removed.

davidlatwe commented 6 years ago

What do you think about instead overriding the call to .data["families"] such that it always returns all families, including family?

I like this idea !

instance.data.get("families") and instance.data["families"] should getting the same result which will all include family right ?

Ummm.. But is it okay if someone already use it to assemble the list of all families ? like..

family = instance.data["family"]
families = [family] if family else []
if "families" in instance.data:
    # would have duplicated family in list if instance.data["families"] include "family"
    families += instance.data["families"]
davidlatwe commented 6 years ago

Will keep thinking on this, and head back tomorrow :) Thank you @mottosso.

davidlatwe commented 6 years ago

I think maybe the best is wait for the transitioning over, since this could be automatically solved after that.

This should be another issue, but is there a TODO list about this transition ?

mottosso commented 6 years ago

No, your right; what I'm proposing would break backwards compatibility..

I've thought about your solution here, and I think you're right. It's such a critical aspect of an instance, that it warrants its own property. It maintains backwards compatibility too, so we could give it a try (in a beta) and roll back if it doesn't jive.

I'll need to call on some of my peers for input, @tokejepsen and @BigRoy what are your thoughts?

BigRoy commented 6 years ago

I kind of like the property the way David implemented it, but it's definitely different from other implementation of things.

Or just remove the usage of family and only support families so things are simplified? And then break backwards compatibility

I wouldn't let instance.data ["families"] behave different, just too confusing.

mottosso commented 6 years ago

The 1 Pyblish Commandment

  1. Thou'st shall not break'eth the' backwards compatibility
mottosso commented 6 years ago

What we could do though, is only document families, and internally convert the now undocumented family member.

That's almost what's happening already, and there isn't any need for the user of Pyblish to know that there exists a family member. That way, @davidlatwe, you could simply stick with families in your plug-ins, and don't bother with family. You shouldn't notice any difference.

davidlatwe commented 6 years ago

What we could do though, is only document families, and internally convert the now undocumented family member.

Sounds like a gentle way to solve this, but to be clear :P, I think you meant key in instance.data, not the member in Plugin ?

If so, by internally convert, you mean like, changing the entity Instance from this

self._data["family"] = "default"

To:

self._data["families"] = ["default"]

And do self._data["families"] = [family] if someone setting .data["family"] or doing update()

something like that ?

mottosso commented 6 years ago

Yes, I meant the key.

By internally, I mean when e.g. QML uses families, it goes ahead and adds family if it exists. We don't need to pre-process anything or change the users' .data.

davidlatwe commented 6 years ago

Ah, then I should alter the plugin that handles instances creation instead. (which takes Maya objectSet attrib "family" and directly update into .data, the one I have was form Avalon)

And should start to forget the existence of data["family"], use data["families"] in the first place when we writing plugins.

Is that correct ?

mottosso commented 6 years ago

Yeah, at least that's one way solve it that doesn't involve changing Pyblish.

Give it a go, maybe there's something I'm not seeing yet.

davidlatwe commented 6 years ago

Ok ! Thanks guys.

Making a conclude here for looking back :

Transitioning from Instances' data["family"] to data["families"]
  1. Not breaking the backwards compatibility
  2. Don't pre-process anything or change the users' .data for preventing confusing.
  3. Starting to forget data["family"] when using Pyblish or writing Plugins, use
    instance.data["families"] = ["my.family"]

    and

    context.create_instance(name=MyFamily, families=["my.family"])
tokejepsen commented 6 years ago

Sorry, I may not be getting where this conversation is heading, but I liked the idea @mottosso had of instance.data["families"] returning all families including family. This will duplicate the family in my plugins, but what is the problem with a duplicated family? As far as I see it, its a decent data duplication for the transition.

davidlatwe commented 6 years ago

what is the problem with a duplicated family?

Oh, I was imaging maybe someone using that self-made families list to run a for loop within Plugins, and might doing same stuff twice because of this.

But might be a rare case though :P

BigRoy commented 6 years ago

Sorry, I may not be getting where this conversation is heading, but I liked the idea @mottosso had of instance.data["families"] returning all families including family. This will duplicate the family in my plugins, but what is the problem with a duplicated family? As far as I see it, its a decent data duplication for the transition.

The thing is that setting the data behaves different than what you retrieve. I'm seeing data as an ordinary dictionary, so it behaving differently for the families key sounds odd to me - almost giving it unexpected behavior.

I'd be fine to start avoiding the usage of family everywhere in favor for families and just force the new behavior myself as long as pyblish itself can fully operate with that.

mottosso commented 6 years ago

Both valid points; this is why I ask you guys. :)

I think for now, let's leave it as-is and try working with only families until we find where that doesn't work.

BigRoy commented 6 years ago

We're trying to switch to solely working with families - we'd prefer that, yet I don't believe the latest mix of Pyblish base master and pyblish qml master currently support that. Maybe QML still needs the "family" to operate?

tokejepsen commented 6 years ago

One issue with pyblish-qml is that it uses the family to label the sections, which would need a rethink.

mottosso commented 6 years ago

QML should be using the first family, if not then that's a bug.

BigRoy commented 6 years ago

So the instance takes the family from the family data after which only for displaying it's joined as concatenated string for all families.

It should instead fall back to families[0] if not family which I don't see it doing. Sounds like the bug is indeed present then. It should first do this defined at line 393 and 394 and then take families[0] as family.

So the only thing that isn't working (looking at this code) is the Sections.

hannesdelbeke commented 3 years ago

referenced this PR in my forum post update: family is legacy and should not be used. now use families!