pyblish / pyblish-base

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

use os.path.join to compose default plugin path #329

Closed davidlatwe closed 6 years ago

davidlatwe commented 6 years ago

Change to os.path.join for cross-platform.

davidlatwe commented 6 years ago

Sham on me, wasn't think this right.

If changes to use os.path.join for composing default plugin path, then will need to have os.path.normpath to do path normalize, for someone already using backslash for default path's register/deregister (which is working but will not after this PR).

but are you sure normpath is what you are looking for then?

Now I think it's "Yes" for sure, both / and \ could work as file path separator on Windows, so it did need normpath to make sure that these two file path strings

path_1 = "server/plugins"
path_2 = r"server\plugins"

are the same workable path.

But on Linux, only / could use as file path separator, \ could be part of the file or folder name, so those two file path strings above won't be recognized as same workable path. First one is a folder path for sure, but the second one could be a file name or else, so it's already a false input when on Linux.

Or at least a similar backslash such that you can register/deregister without thinking about it.

I think this did solve for Windows, but wasn't needed for Linux.

Could verify with this ```python import os input_1 = "server/plugin" input_2 = r"server\plugin" path_1 = os.path.join(os.getcwd(), os.path.normpath(input_1)) path_2 = os.path.join(os.getcwd(), os.path.normpath(input_2)) try: os.makedirs(path_1) print("path_1 created.") except OSError: print("path_1 already exists.") try: os.makedirs(path_2) print("path_2 created.") except OSError: print("path_2 already exists.") # Windows # path_1 created. # path_2 already exists. # Linux # path_1 created. # path_2 created. ```


Then, in the Doctest, maybe there won't need two platform type of tests ? Since we won't need to test this r"server\plugins" on a Linux machine, because it's not a file path for Linux at all.

So I changed as follow

"""
>>> import os
>>> my_plugins = os.path.join("server", "plugins")
>>> register_plugin_path(my_plugins) == os.path.normpath(my_plugins)
True
"""

Does this make sense ?

mottosso commented 6 years ago

Then, in the Doctest, maybe there won't need two platform type of tests ? Since we won't need to test this r"server\plugins" on a Linux machine, because it's not a file path for Linux at all.

Yes, I think you're right. When you register these, you should register them using the slash of the active platform, e.g. via os.path.join(). In which case this will work just fine.

I think this is good, it's definitely an improvement. Happy to merge this, if you could bump this to 1.5.6 in version.py then I'd be happy to merge this!

davidlatwe commented 6 years ago

Thanks ! Right away :)

mottosso commented 6 years ago

Thank you. :)