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

Extend plugin sorting logic #369

Closed
wants to merge 3 commits into from
Closed

Extend plugin sorting logic #369

wants to merge 3 commits into from

Conversation

buddly27
Copy link
Contributor

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

This resolves #368

Ensure that ContextPlugin are sorted before InstancePlugin when plugins have the same order
@mottosso
Copy link
Member

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
Copy link
Contributor Author

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:

  • Plugin path registration order (here)
  • os.listdir (here)
  • order of plugins within a module (here)
  • Plugin registration order (here)

… code

If PYBLISH_SORT_PER_ORDER_AND_TYPE is set, plugins will be sorted per order and per type.
PYBLISH_EARLY_ADOPTER will also apply the new plugin sorting rule.
@buddly27
Copy link
Contributor Author

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.

@buddly27 buddly27 closed this by deleting the head repository Jul 18, 2023
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.

Ensure that ContextPlugins are sorted before InstancePlugins for similar order
2 participants