pyblish / pyblish-base

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

plugin path normalize #328

Closed davidlatwe closed 6 years ago

davidlatwe commented 6 years ago

Two simple improvement as follow :

  1. use os.path.join instead of hard-coded slash to compose default plugin path.
  2. use os.path.normpath to normalize plugin path before register/deregister it.
davidlatwe commented 6 years ago

Ummm, not familiar with this. Don't know why travis-ci failing the doctest but appveyor succeeded.

Was trying to make the tests to except the normalized path as result.

mottosso commented 6 years ago

Travis is Linux, AppVeyor is Windows.

I've updated the doctest, this might work.

davidlatwe commented 6 years ago

How about this ?

"""
        >>> import os
        >>> # Linux
        >>> register_plugin_path("server/plugins") == os.path.join("server", "plugins")
        True
        >>> # Windows
        >>> register_plugin_path(r"server\plugins") == os.path.join("server", "plugins")
        >>> # Already registered because normalized.
        False
"""
mottosso commented 6 years ago

Ah! Doi. My bad. Having another look. :)

davidlatwe commented 6 years ago

This gonna work !

mottosso commented 6 years ago

Looks like we've found a bug. Deregister will need normpath as well.

davidlatwe commented 6 years ago

But I did add it there, too :(

davidlatwe commented 6 years ago

Turns out that, in Windows

>>> os.path.normpath("server/pluginsB")
'server\\pluginsB'
>>> os.path.normpath(r"server\pluginsB")
'server\\pluginsB'

but in Linux

>>> os.path.normpath("server/pluginsB")
'server/pluginsB'
>>> os.path.normpath(r"server\pluginsB")
'server\\pluginsB'

phew...

mottosso commented 6 years ago

Ah, but are you sure normpath is what you are looking for then? I think what we're looking for is something to replace any backslash with the right one for the platform. Or at least a similar backslash such that you can register/deregister without thinking about it. This doesn't solve anything?

davidlatwe commented 6 years ago

I think what we're looking for is something to replace any backslash with the right one for the platform.

The normpath did make Windows platform easier passing path, no matter backslash or not, however Linux could use backslash as part of folder name...

This doesn't solve anything?

Me personally think that normpath could be like a simple guard for _registered_paths. But assume most of the case we just copy the path which registered previously, and paste to deregister, so maybe this is unnecessary, only the first commit in this PR is needed. :(

davidlatwe commented 6 years ago

How about not to do normpath in this PR, but making deregister_plugin_path() to print out what path it tried to remove when failed ? Not just return x not in list error. ~current registered paths if failed to remove one ?~

Easier to debug if in an automation process.

davidlatwe commented 6 years ago

It's off this PR's original purpose now, it was at first try to normalize folder path's separator for plugin path's registration more straight forward, but turns out it doesn't have a simple straight answer when dealing cross platform, at least I can't think of any reliable way.

Secondly, I believe that this wasn't Pyblish's responsibility need to take care of, just making more convenient, so if it doesn't have a simple answer, then this PR has failed.

I will close this PR, and make it cleaner. Sorry about that. :sneezing_face: