-
Notifications
You must be signed in to change notification settings - Fork 58
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
Feature - Added callback pluginProcessed #292
Conversation
…rocessed, regardless of the result. added to plugin.process function
After making a complete and utter mess at trying to rebase the previous fork, I gave up and accepted failure, (was doing that to fix my incorrectly authored commits.) So unfortunately I have re-forked and committed again so we loose the conversation of the previous pull request. Adding in some of the comments from last time by @mottosso: Linking to the corresponding Forum thread. http://forums.pyblish.com/t/pluginprocessed-callback/264 Secondly, I don't think you need all of the keyword arguments, but instead only the result=, since it will in turn contain reference to the corresponding plugin, context and instance. Thirdly, could you also make sure this applies to implicit plug-ins as well? You are welcome to put this signal in the main process() function if you wish. Finally, there will need to be one or more tests to exercise all of what you expect of the feature. About updating the API docs, you are most welcome to do so. |
I have addressed points 1 - 3. Still need to create tests and update docs. |
…sn't seem to be working
Hi Marcus
thanks |
Hey @pscadding, no problem, good to see you're back at it. To run tests, you're doing it just right. $ # ensure pyblish-base is available on your PYTHONPATH
$ python run_testsuite.py And about the failure, it looks like your test isn't actually installing any plug-ins for |
Ah yeah, cool I wondered how it was working. Well makes sense, I've updated it will a couple of dummy plugins to test it with. It now seems to test correctly and with out failure. I still need to update the documentation. |
If you have a look at the function these tests are running, you'll see that they aren't emitting signals from a plug-in, but rather from within the function call itself. Great work! Looks like we might be able to merge this soon! |
Documentation updated: and Thank for you for you help and patience. |
Ah of course. Yeah makes sense that those two events wouldn't require that. However I can't find an example of a test for pluginFailed, which would presumably require a plugin to be provided. |
Good catch, I think you've found a missing test. We use coverage.py to get an overview of what isn't yet tested. It isn't perfect, and it looks like this one slipped through the cracks. Here you can see where it appears to be run at least 3 times during the tests, but it doesn't necessarily mean there is a test for this one line of code. You can run this locally as well and get a webpage generated with a similar report. It tells you which lines of code where run during tests and which weren't. This is the You are welcome to add a test for this signal as well, otherwise an issue so we can keep track of it. Thanks for the sharp eyes! |
AppVeyor failed, but turns out it was a fluke on their end, not ours. I re-ran the build and it succeeded. This looks good! If you're happy, I'm happy to merge this now. Let me know if there's anything else you would like to get in. |
I added a pluginFailed test last night, although it doesn't currently verify all the arg types its being passed. If your happy enough with how it is then, yeah I'm happy for it to be merged. There's nothing else I want add to it. |
|
||
def on_failed(plugin, context, instance, error): | ||
#todo: add further checks for the other incoming args | ||
#assert isinstance(instance, CheckInstanceFailRaise) |
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.
Ah, yes I see you mean these comment-out ones. How come they didn't get included?
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 couldn't get the isinstance to work with the plugin and instance.
I tried isinstance(instance, pyblish.api.InstancePlugin)
but that didn't work and I also tried comparing to directly to the class defined in the test, but that didn't seem to work.
It needs a bit more looking into.
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.
Ah, yes the plug-in you're getting is probably not an instance, but the class itself. Try something like issubclass(plugin, pyblish.api.InstancePlugin)
or simply plugin == CheckInstanceFail
.
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.
Oh, and with isinstance(instance, pyblish.api.InstancePlugin)
, the instance
isn't the plug-in, but the instance you create via context.create_instance
. You can compare it with pyblish.api.Instance
.
Thank you for the code. |
Thanks @pscadding! |
Great work @pscadding! Awesome to see a new contributor:) |
Added a callback that gets emited everytime a plugin gets processed, regardless of the result.
added to plugin.process function