-
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
Ensure that ContextPlugins are sorted before InstancePlugins for similar order #368
Comments
It sounds like a solid convention, but it wouldn't be possible to change this logic as it would break several years of plug-ins written without it. It's also possible it doesn't fit with everyone's preference. In this case, I would establish this convention internally, for example..
import pyblish.api
import mypipeline
class Collect(pyblish.api.ContextPlugin):
"""Context plugin to create dummy instances."""
order = mypipeline.get_collector_order() # => CollectorOrder + 0.0
label = "Create dummy instances"
class Construct(pyblish.api.InstancePlugin):
"""Instance plugin to initiate data for each instances."""
order = mypipeline.get_collector_order(0.1) # => CollectorOrder + 0.6
label = "Initiate default data" Another alternative may be to establish a |
Admittedly I don't see a reason to change this. I think We do have quite some ContextPlugins running over some instances so they are in-between of InstancePlugins too. The ordering on a single order value I consider random which is fine... if I want a specific order I'd do +0.1 or so. |
I agree with @BigRoy. Better to be explicit than implicit here. I would say though that implementing adding the ability to register your own custom sorting function, would be a nice addition. |
I'm not sure I understand how this could break any pipeline though, all I'm suggesting is to add a rule when there are no rules: Before:
plugins.sort(key=lambda plugin: plugin.order) After:
plugins.sort(key=lambda plugin: (plugin.order, _sort_by_type(plugin))) Also I don't see how this would make the sorting logic less explicit.. Or am I missing something? |
That's true, I see what you mean now. I think we all thought that ContextPlugin's would come before InstancePlugins no matter what. In that case, the first step is putting this in a PR. That will trigger the automated tests on all platforms, and if those work then that'll be a good indicator nothing supported has broken. |
Just submitted the PR, previous tests seems to work fine and I added an extra one to ensure that it behaves as expected. Let me know what you think! |
In most cases, it seems that the convention is to use ContextPlugin for the Collection stage and InstancePlugin for the following stages. However there might be some exception, for instance the InstancePlugin could be used in the Collection stage to initialize configuration of newly created instances.
For instance:
The
order
attribute is important to ensure that the InstancePlugin is always picked up after the ContextPlugin. But thinking about it more, I feel like it never really makes sense to have an InstancePlugin before a ContextPlugin, so I was wondering if there could be a more elegant solution to ensure that for plugins with the same order, ContextPlugin would always be sorted before InstancePlugin.I thought about extending the sorting logic as follows:
What do you think?
The text was updated successfully, but these errors were encountered: