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 api.create able to run with one specific creator plugin #531

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

Conversation

davidlatwe
Copy link
Collaborator

@davidlatwe davidlatwe commented Mar 10, 2020

Changes

This PR closes #507. Depends on the type of input argument family of api.create, try matching all plugins via family if it's a string, or dierctly run the given plugin if family is that plugin.

@davidlatwe davidlatwe requested a review from BigRoy March 10, 2020 04:04
@davidlatwe
Copy link
Collaborator Author

Here's my test code

class CreateA(api.Creator):
    label = "A"
    family = "foobar"

    def process(self):
        print("A")

class CreateB(api.Creator):
    label = "B"
    family = "foobar"

    def process(self):
        print("B")
        

from avalon import api
from avalon.tools import creator
# Deregister all plugins for clarity
for path in api.registered_plugin_paths()[api.Creator]:
    api.deregister_plugin_path(api.Creator, path)

api.register_plugin(api.Creator, CreateA)
api.register_plugin(api.Creator, CreateB)

creator.show()

After this PR, only one creator obove will be triggered.

Copy link
Collaborator

@BigRoy BigRoy left a comment

Choose a reason for hiding this comment

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

Solid work @davidlatwe - admittedly seeing family being used as plug-in directly without a clear commet regarding it got me confused for a second. Added a comment to maybe tweak that code layout a bit for readability. With that tweaked, let's merge this!

Comment on lines 881 to 884
if isinstance(family, str):
Plugins = [P for P in discover(Creator) if family == P.family]
else:
Plugins = [family]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This got me wondering for a second what is going on here. Should this maybe be in reverse, something like this:

    if isinstance(family, Creator):
        # Allow passing in a specific Creator to run only that plug-in
        Plugins = [family]
    else:
        Plugins = [P for P in discover(Creator) if family == P.family]

Also makes the check more explicit to it being a Creator plug-in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point !
Changed in commit ebcb78c
( note that the family has now changed to also accept a subclass of api.Creator, so it should be using issubclass ;) )

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well spotted!

@davidlatwe
Copy link
Collaborator Author

Merging this today :D

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 30, 2020

@davidlatwe I'm wondering. Since you've a ready to go test for this? Any chance you can add a test too for the unittests?

One that tests the old by string family behavior, and the new behavior? Just to make sure we preserve it well for the future - or at least know about any potential changes.

It would be in core/avalon/tests then.

@davidlatwe
Copy link
Collaborator Author

Just added, also fixed an obvious bug that I missed in ebcb78c. :P

@davidlatwe
Copy link
Collaborator Author

Weird, the test actually failed, but build passed. :/

The test passed in my Maya session, maybe I should write the test plugin into an actual script file.

@davidlatwe
Copy link
Collaborator Author

Ah, now I see the problem, the host registed in test_pipeline has no attribute 'maintained_selection'

avalon.pipeline: WARNING: 'module' object has no attribute 'maintained_selection'

@davidlatwe
Copy link
Collaborator Author

Test fixed ! See log

@BigRoy
Copy link
Collaborator

BigRoy commented Sep 30, 2020

Awesome work!

The only thing you are missing in the test is the test case where two plug-ins register to the same family where running it by family string name it should run both (as originally) and by solely Plugin run only the single one. That's the test case we want to preserve over time I suppose (or at least know about that it changes).

# psuedocode

data= {"value": 0}

class CreateA(api.Creator):
    label = "A"
    family = "foobar"

    def process(self):
        data["value"] += 1

class CreateB(api.Creator):
    label = "B"
    family = "foobar"

    def process(self):
        data["value"] += 10
        

from avalon import api
from avalon.tools import creator
# Deregister all plugins for clarity
for path in api.registered_plugin_paths()[api.Creator]:
    api.deregister_plugin_path(api.Creator, path)

api.register_plugin(api.Creator, CreateA)
api.register_plugin(api.Creator, CreateB)

api.create("foo", "my_asset", family="foobar")
assert data["value"] == 11, "Must run both Creator Plugins"

api.create("foo", "my_asset", family=CreateA)
assert data["value"] == 12, "Must run onlyCreatorA Plugin"

Following counting-like logic from pyblish tests, example.

@davidlatwe
Copy link
Collaborator Author

That indeed much better ! Thanks. 🚀

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.

Use specific Creator Plugin when multiple Creators exist for single family
2 participants