pyblish / pyblish-base

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

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

Closed hannesdelbeke closed 2 years ago

hannesdelbeke commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

hannesdelbeke commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Let's do it!

mottosso commented 2 years ago

Oh you versioned up as well, splendid. :D

BigRoy commented 2 years ago

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

hannesdelbeke commented 2 years ago

might have, i'll look into that

hannesdelbeke commented 2 years ago

yes it was missed out @BigRoy , good spot. it fails the test i just wrote. here is a new PR https://github.com/pyblish/pyblish-base/pull/384