Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 16 commits into from
Oct 27, 2021
Merged

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

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0baed4f
support- new path type in-plugin-registration by converting to string…
hannesdelbeke Oct 25, 2021
d5fecd3
pathlib test
hannesdelbeke Oct 26, 2021
3772887
add missing unittest import
hannesdelbeke Oct 26, 2021
3127cc2
skip update
hannesdelbeke Oct 26, 2021
286506c
update the tester, helper func was picked up by the test framework
hannesdelbeke Oct 26, 2021
ca53e03
add encoding
hannesdelbeke Oct 26, 2021
b43e0f7
add to assert message
hannesdelbeke Oct 26, 2021
94a7cf0
add unicode encoding
hannesdelbeke Oct 26, 2021
27116a1
add pathlib support for python 3.4+
hannesdelbeke Oct 26, 2021
09afb18
python 2 NotImplementedError to importError
hannesdelbeke Oct 26, 2021
91bbb2e
add normpath
hannesdelbeke Oct 26, 2021
803df44
fix errors in except
hannesdelbeke Oct 26, 2021
5f3f2be
add /t to test more string stuff
hannesdelbeke Oct 26, 2021
23ddb78
remove binary string from test
hannesdelbeke Oct 26, 2021
7e5c2ff
disabling binary string
hannesdelbeke Oct 26, 2021
0dbd091
implement feedback
hannesdelbeke Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions pyblish/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,14 @@ def register_plugin_path(path):

"""

# accept pathlib paths and convert to string
try:
import pathlib
if isinstance(path, pathlib.PurePath):
path = str(path)
except ImportError:
pass

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hannesdelbeke hannesdelbeke Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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”.

Copy link
Contributor Author

@hannesdelbeke hannesdelbeke Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

normpath = os.path.normpath(path)
if normpath in _registered_paths:
return log.warning("Path already registered: {0}".format(path))
Expand Down
40 changes: 40 additions & 0 deletions tests/test_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#!/usr/bin/python
# -*- coding: utf-8 -*-

import os
import logging

Expand All @@ -13,8 +16,15 @@
raises,
)

try:
import pathlib
except:
pathlib = None

from . import lib

import unittest


@with_setup(lib.setup_empty, lib.teardown)
def test_unique_id():
Expand Down Expand Up @@ -490,6 +500,36 @@ class MyPlugin(pyblish.plugin.Collector):
pyblish.plugin.register_plugin(MyPlugin)



@unittest.skipIf(pathlib is None, "skip when pathlib is not available")
def test_register_plugin_path():
"""test various types of input for plugin path registration"""

# helper function
@with_setup(lib.setup_empty, lib.teardown)
def helper_test_register_plugin_path(path):
pyblish.plugin.register_plugin_path(path)
registered_paths = pyblish.api.registered_paths()
path = os.path.normpath(str(path))
assert path in registered_paths, path + ' not in ' + str(registered_paths) # check if path in here

# create input
input_to_test = []
from pathlib import Path, PurePath, PureWindowsPath, WindowsPath, PosixPath, PurePosixPath
path_types = [Path, PurePath, PureWindowsPath, WindowsPath, PosixPath, PurePosixPath]
for path_type in path_types:
try:
input_to_test.append(path_type('test/folder/path')) # create pathlib input
except NotImplementedError: # PosixPath can't be instantiated on windows and raises NotImplementedError
pass
# input_to_test.append(u"c:\some\special\södär\testpath".encode('utf-8')) # create unicode input
# input_to_test.append(b"c:\\bytes\\are/cool") # create bytestring input

# test all paths from input
for path in input_to_test:
helper_test_register_plugin_path(path)


@mock.patch("pyblish.plugin.__explicit_process")
def test_implicit_explicit_branching(func):
"""Explicit plug-ins are processed by the appropriate function"""
Expand Down