-
Notifications
You must be signed in to change notification settings - Fork 59
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
base: master
Are you sure you want to change the base?
Conversation
commit 60f817b, I was referencing from this stackoverflow answer |
Thanks for this @davidlatwe, my only concern is that You're right, that currently because we are in a transition period of having both What do you think about instead overriding the call to |
I like this idea !
Ummm.. But is it okay if someone already use it to assemble the list of all families ? 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"] |
Will keep thinking on this, and head back tomorrow :) |
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 ? |
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? |
tests/test_plugin.py
Outdated
context = pyblish.plugin.Context() | ||
instance = context.create_instance("MyInstance") | ||
instance.data["family"] = "A" | ||
families = list(instance.data["family"]) |
There was a problem hiding this comment.
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"]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch ! Right !!
tests/test_plugin.py
Outdated
assert_equals(families, instance.families) | ||
|
||
instance.data["families"] = ["B", "C"] | ||
families.extend(instance.data["families"]) |
There was a problem hiding this comment.
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"]
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 I wouldn't let |
The 1 Pyblish Commandment
|
What we could do though, is only document That's almost what's happening already, and there isn't any need for the user of Pyblish to know that there exists a |
Sounds like a gentle way to solve this, but to be clear :P, I think you meant key in If so, by internally convert, you mean like, changing the entity self._data["family"] = "default" To: self._data["families"] = ["default"] And do something like that ? |
Yes, I meant the key. By internally, I mean when e.g. QML uses |
Ah, then I should alter the plugin that handles instances creation instead. (which takes Maya objectSet attrib "family" and directly update into And should start to forget the existence of Is that correct ? |
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. |
Ok ! Thanks guys. Making a conclude here for looking back : Transitioning from Instances'
|
Sorry, I may not be getting where this conversation is heading, but I liked the idea @mottosso had of |
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 |
The thing is that setting the data behaves different than what you retrieve. I'm seeing I'd be fine to start avoiding the usage of |
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 |
We're trying to switch to solely working with |
One issue with pyblish-qml is that it uses the |
QML should be using the first family, if not then that's a bug. |
So the instance takes the It should instead fall back to So the only thing that isn't working (looking at this code) is the Sections. |
referenced this PR in my forum post |
I think this could provide a bit more convenience.
Instead of querying
family
andfamilies
frominstance.data
and adding them up every time we need that info, making that as a built-in would be nice, right ?Happy new year :)