Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

davidlatwe
Copy link
Contributor

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
Copy link
Contributor Author

commit 60f817b, I was referencing from this stackoverflow answer

@mottosso
Copy link
Member

mottosso commented Jan 8, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@davidlatwe
Copy link
Contributor Author

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
Copy link
Member

mottosso commented Jan 9, 2018

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?

context = pyblish.plugin.Context()
instance = context.create_instance("MyInstance")
instance.data["family"] = "A"
families = list(instance.data["family"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're lucky; this would actually make each character of the family into individual members of a list. :)

>>> list("abc")
['a', 'b', 'c']
>>>

Better make it..

families = [instance.data["family"]]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch ! Right !!

assert_equals(families, instance.families)

instance.data["families"] = ["B", "C"]
families.extend(instance.data["families"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always found this syntax a little more readable.

families += instance.data["families"]

@BigRoy
Copy link
Member

BigRoy commented Jan 9, 2018

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
Copy link
Member

mottosso commented Jan 9, 2018

The 1 Pyblish Commandment

  1. Thou'st shall not break'eth the' backwards compatibility

@mottosso
Copy link
Member

mottosso commented Jan 9, 2018

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
Copy link
Contributor Author

davidlatwe commented Jan 9, 2018

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
Copy link
Member

mottosso commented Jan 9, 2018

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
Copy link
Contributor Author

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
Copy link
Member

mottosso commented Jan 9, 2018

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
Copy link
Contributor Author

davidlatwe commented Jan 9, 2018

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
Copy link
Member

tokejepsen commented Jan 9, 2018

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
Copy link
Contributor Author

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
Copy link
Member

BigRoy commented Jan 9, 2018

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
Copy link
Member

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
Copy link
Member

BigRoy commented Feb 20, 2018

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
Copy link
Member

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

@mottosso
Copy link
Member

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

@BigRoy
Copy link
Member

BigRoy commented Feb 20, 2018

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
Copy link
Contributor

hannesdelbeke commented Oct 8, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants