search

pyblish / pyblish-base

Pyblish base library - see https://github.com/pyblish/pyblish for details.
Other
127 stars 59 forks source link

Extend plugin sorting logic #369

Closed buddly27 closed 1 year ago

buddly27 commented 4 years ago

Ensure that ContextPlugin are sorted before InstancePlugin when plugins have the same order.

This resolves #368

mottosso commented 4 years ago

Thanks for this, it does look fine as far as tests go, which is a plus.

The only thing left to consider is whether it will make debugging more or less difficult in the future.

Having ContextPlugin before InstancePlugin isn't necessarily intuitive when compared to, say, an alphabetical order based on plug-in name.

Which reminds me, and I'd have to check, but it's possible that because plug-ins are coming from disk (e.g. os.listdir) in an alphabetical order they remain alphabetical once sorted with identical order numbers. That sorting just doesn't change them. And if so, this change would break up the alphabetical order which could be unexpected and hard to narrow down the cause of.

Here's what I think we could do. Currently, there are a few changes made to Pyblish that break compatibility in similar, subtle ways. And they've been put behind an environment variable where they can be "innocent until proven guilty", so to speak. They enable anyone to use Pyblish as though the feature was persistent, without affecting those that do not need it.

If we could make this change only apply to those with the PYBLISH_SORT_CONTEXTPLUGINS_FIRST environment variable set, then it would be safe to merge immediately, and for anyone to evaluate. Then once it's been deemed safe, we could remove the variable and make it permanent.

Then you could also include it under the PYBLISH_EARLY_ADOPTER flag as well, so it applies to those who go all-in. The idea is to make each of those permanent on the next major version change, 2.0.

Let me know what you think.

buddly27 commented 4 years ago

Yeah that sounds like a good idea. I'm gonna update the PR accordingly.

Having ContextPlugin before InstancePlugin isn't necessarily intuitive when compared to, say, an alphabetical order based on plug-in name.

Yeah it would definitely be an additional convention that might not suit everyone. We might as well add a way to register a different sorting logic as you suggested in the related issue? Would you like me to add this in the PR?

Which reminds me, and I'd have to check, but it's possible that because plug-ins are coming from disk (e.g. os.listdir) in an alphabetical order they remain alphabetical once sorted with identical order numbers. That sorting just doesn't change them. And if so, this change would break up the alphabetical order which could be unexpected and hard to narrow down the cause of.

Hmm not sure about this logic, order resulting from os.listdir is an artifact of the filesystem and cannot be relied upon, so you cannot really expect alphabetical order.

Also the current order before sorting is not only dependent on os.listdir, it depends on:

buddly27 commented 4 years ago

Plugin updated as you suggested. I named the environment variable PYBLISH_SORT_PER_ORDER_AND_TYPE instead of PYBLISH_SORT_CONTEXTPLUGINS_FIRST to not mislead people by implying that the original sorting-per-order logic is removed.