support- new path type in-plugin-registration by converting to string…

[hannesdelbeke]

add support for the new "path" type from python 3's build-in std-lib in plugin path registration
(can also be used in python 2 if you install pathlib)

a Path can be joined with other strings, and removes the need for using os.join etc resulting in a lot cleaner code
bringing support to pyblish results in much cleaner code when using paths.
and paths is the way forward after the big support it received in python 3.

when converted to a string it will automatically select the correct \ or / based on OS and return a str path

[mottosso]

Nice and clean :) However, how does this react to unicode and bytestrings?

path = u"c:\some\special\södär\path"
path = b"c:\\bytes\\are/cool"

Would it be safer to query specifically for this new path type?

if type(path).__name__ == "path":
  path = str(path)

Needs tests for these cases.

[hannesdelbeke]

was thinking of doing it that way but that would only work if you have pathlib installed

import pathlib  #(or pathlib 2 in python 2)
if isinstance(path, pathlib.Path):

so I guess we put this in a try except ?

also same with the test, is there a way the tester knows which python version it runs?
would need to test when pathlib exists and when it doesnt.

[mottosso]

was thinking of doing it that way but that would only work if you have pathlib installed

You can test against unavaialble types using type(path).__name__ == "Path".

also same with the test, is there a way the tester knows which python version it runs?

You can do e.g.

if sys.version_info[0] == 3:
  def test_for_python3():
     ...

Or..

try:
  import pathlib
  def test_when_pathlib_exists():
    ...
except ImportError:
  pass

[mottosso]

Have a look here at how you can run these tests on you local machine.

• https://github.com/pyblish/pyblish-base/blob/master/.travis.yml#L10

[hannesdelbeke]

ok so have an implementation and a test, but don't get why this is happening
b'c:\some\special\s\xc3\xb6d\xc3\xa4r\path' becomes
b'c:\\some\\special\\s\xc3\xb6d\xc3\xa4r\\path' when added to registered paths
as far as i can see all that happens in register_plugin_path is os.path.normpath, which i also run in my test

annoyingly i can't get the tests to run locally correctly so have to rely on checks from appveyor which is slow.
likely because i have so many version of python and different environments, and need to isntall nose

well just as i posted this you posted about local tests above, will have a look

[hannesdelbeke]

it get's worse
b'c:\some\special\s\xc3\xb6d\xc3\xa4r\testpath' becomes
b'c:\\some\\special\\s\xc3\xb6d\xc3\xa4r\testpath' when added to registered paths
notice the \ before testpath is a single one, but all other folder \ are 2 now

the annoying thing is i can see it's working, just spending so much time on making this test work.
i could only run the command and not confirm the data afterwards in the test,
in that case we test if it works without crashing. which is what mosts pyblish tests seem to do

[hannesdelbeke]

TBH this seems to be an issue with non ascii binary string, and not the pathlib paths.
it should be a separate test, i'll remove this one for now

[hannesdelbeke]

seems to work and test passes for pathlib, opened a new issue to discuss the unicode and bytestring changes since this happens unrelated to the pathlib code

[mottosso]

This is a mess. :S

1. A try/except for a single type of path, it's too close to looking like legacy code.
2. str(path) is too brute-force. Odds are the library evolves to pretty-print the path on str()

If we really must special-case pathlib, then maybe we could duck-type it instead?

if hasattr(path, "as_posix"):
  path = path.as_posix()

That way, we don't need any imports and we account for any other library that follows a similar convention to pathlib of having a as_posix() call that converts an object into something posix compliant; i.e. a plain well-formatted path as a string.

[hannesdelbeke]

thanks Mottosso, you are right it is a mess.
your if stament looks a lot cleaner

if hasattr(path, "as_posix"):`

but not sure if the second part will work, since we now always convert to posix. always returning forward slashes (/)

  path = path.as_posix()

about str(), the docs for pathlib say:

The string representation of a path is the raw filesystem path itself (in native form, e.g. with backslashes under Windows), which you can pass to any function taking a file path as a string:

if hasattr(path, "as_posix"):`
  path = str(path)

would be more correct I believe since this would choose the correct slashes based on your OS when using Path, and maintain the slashes of your pathtype if passing specific types of path. instead of always returning posixpath style slashes

or we could go with the first suggestion you had and compare with all pathtype names

if type(path).__name__ in ['Path', 'PurePath', 'PureWindowsPath', 'WindowsPath', 'PosixPath', 'PurePosixPath']:
  path = str(path)

i went with the try except because it would support any new pathtypes in future pathlib upgrades. and python's idiom "it's easier to ask for forgiveness than permission”.

[hannesdelbeke]

ideally we would not convert to string and use paths directly,
and convert any string to paths.

but then pyblish would rely on pathlib which introducing dependency issues instead for python 2 and <3.4
+the amount of work is a lot more making this not realistic

quick comparison: if we take this part of the discover function

    # Include plug-ins from registered paths
    for path in paths or plugin_paths():
        path = os.path.normpath(path)
        if not os.path.isdir(path):
            continue

        for fname in os.listdir(path):
            if fname.startswith("_"):
                continue

            abspath = os.path.join(path, fname)

            if not os.path.isfile(abspath):
                continue

            mod_name, mod_ext = os.path.splitext(fname)

            if not mod_ext == ".py":
                continue

rewritten with pathlib would be something like

    # Include plug-ins from registered paths
    for path in paths or plugin_paths():
        if not path.is_dir():
            continue

        for abspath in path.iterdir():
            if abspath.name.startswith("_") or \
            not abspath.is_file() or \
            not abspath.suffix == ".py":
                continue

[mottosso]

Let's focus on what we want to achieve, which to clarify is being able to do this:

import pathlib
path = pathlib.Path(r"c:\my\plugins")

from pyblish import api
api.register_plugin_path(path)

Is that right? If so, then..

if hasattr(path, "as_posix"):
  path = path.as_posix()

..will get you there with a minimal amount of code that is neither ambiguous, add additional imports or logic and won't need any additional tests. The following call to normpath will translate any slashes to what is appropriate the platform.

[hannesdelbeke]

if normpath does that then all should be good with that approach

[mottosso]

Let's do it!

[mottosso]

Oh you versioned up as well, splendid. :D

[BigRoy]

Did this PR miss out on doing the same for deregister_plugin_path?

[hannesdelbeke]

might have, i'll look into that

[hannesdelbeke]

yes it was missed out @BigRoy , good spot. it fails the test i just wrote. here is a new PR #384