search

pyblish / pyblish-base

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

add support to register plugins by module path #373

Open hannesdelbeke opened 3 years ago

hannesdelbeke commented 3 years ago

TLDR: this PR adds support to register a plugin with the path to a python file(module). allowing you to register individual files without having to change your folder structure.

Description:

atm pyblish only supports registering a plugin either

the paths are also saved in registered paths, and are returned by

i chose to reuse this variable since it requires minimum changes to the codebase, and is a lot cleaner than having a second var. it also reads nice, registered paths contains all registered paths. both file and folder.

this was tested with pyblish QML and lite.

regarding potentially breaking old pipelines/ pyblish setups. since before a file path was not allowed in registered paths. no one will have any workflows with files in the registered plugin paths. this leads to my assumption it will be backwards compatible.

the main function using plugin_paths in the pyblish API is discover. which now handles filepaths correctly. the CLI does not require changes i believe.

incase there is a test that checks if registered paths does not contain a filepath, this test will now fail.

hannesdelbeke commented 3 years ago

just discovered the dev docs so PR likely will need some work https://pyblish.gitbooks.io/developer-guide/content/implementing_a_feature.html

mottosso commented 3 years ago

I've thought about this and can see the use for it.

However, the discover function is at the root of all of Pyblish, and will need tests to account for all possible cases. All of them. On top of that, this change introduces a lot of new complexity to this function which means a ton of room for error. I'm onboard with the idea, but the implementation is currently too complex. I can take a closer look to see whether there is a simpler way of achieving it, but it may take a while. If on the other hand you can think of an alternative, simpler approach then that would be great.

hannesdelbeke commented 3 years ago

fair enough, I'm aware discover is a core func and backwards compatibility is key.

regarding complexity of this PR: if you look at the PR commits instead of at all changed code, it's easier to follow what's going on

the first 2 commits touch a lot of code, but do not change ANY functionality. just a refactor to avoid duplicate code. they move code to helper functions, to remove duplicate code.

this is in preparation for the 3rd commit which introduces the actual new functionality: https://github.com/pyblish/pyblish-base/pull/373/commits/36d9902732491e0bd240a9bdc8ee17e298098869 when only looking at this commit is the implementation still to complex?

hannesdelbeke commented 3 years ago

would it be worth splitting this PR so we can tackle the changes separately?

commit 1 removes duplicate code pyblish has 2 functions with the same code. this commit moves the code to a helper function. no changes in any functionality

commit 2 move the code for discovering a module to it's own function, so it's not embedded in the discover function.

notice it does introduce a small change! instead of passing path, fname, and then later construct abspath we now pass abspath, and later construct path, fname this is likely the place where anything would break if it breaks.

commit 3 is where we add the new functionallity. what happens here is that registered paths now also contains the paths of .py files, instead of folders only. I can't think of anything where it would break backwards comp by introducing this change though.