Dvc: compat: fspath doesn't work with pathlib.Path on Python 3.5

[ghost]

fspath is currently (0.70.0) not working for pathlib.Path in python 3.5 because it lacks __fspath__ implementation.

We need to either fix it, or put a warning on this to prevent using pathlib.Path with fspath.

[efiop]

@mroutis Just to clarify, we don't really try to feed pathlib.Path to fspath in the current project, right? The issue is about the non-merged patch that tries to use it in our config logic.

[ghost]

@efiop , yes, currently we don't have problem with this because we use either PathInfo or the string representation for paths.

PathInfo implements __fspath__ so it works on Python 3.5.

So it is not affecting, nor stopping the current development.

Thanks for the clarification, @efiop :slightly_smiling_face:
This issue was extracted from some discussion on #2826

[ghost]

I'd say that we either close this or address it, since the patch was already implemented:

diff --git a/dvc/utils/compat.py b/dvc/utils/compat.py
index 372c8d2c..4242fc00 100644
--- a/dvc/utils/compat.py
+++ b/dvc/utils/compat.py
@@ -207,6 +207,11 @@ except ImportError:
         except AttributeError:
             if hasattr(path_type, "__fspath__"):
                 raise
+
+            # Support for pathlib.Path in Python 3.5, since it doesn't
+            # implement `__fspath__`
+            if isinstance(path, pathlib.Path):
+                return str(path)
+
             else:
                 raise TypeError(
                     "expected str, bytes or os.PathLike object, "

Thoughts on this one, @iterative/engineering ?
Thoughts in general about using PathInfo, pathlib.Path, strings?

[efiop]

@mroutis That patch only makes sense if it is going to be used somewhere, otherwise it is just dead code. I would combine it with a patch that actually uses it.

[ghost]

So let's close it, since no one is using pathlib.Path, and reopen it if someone needs it.

[Suor]

I am with @mroutis here actually. The thing that we can't fspath(Path(...)) is wrong, additionally Path/PurePath is not os.PathLike in Python 3.5, which makes:

pathlib.Path(pathlib2.Path(...))  # raises exception
pathlib2.Path(pathlib.Path(...))  # raises exception

Which occasionally causes issues with third-party libs expecting or providing Path-like objects, e.g. tmp_path pytest fixture.

[Suor]

A possible solution would be using pathlib2 in Python 3.5 instead of stdlib pathlib. We should, however, be careful since some patches in PathInfo done for Python 2 might actually address pathlib/pathlib2 differences.

[efiop]

@Suor Nice thing about Path is that its str doesn't return relpath as in our PathInfo(it is done for a reason though), so so far we've been simply using str(Path) in tests. I'm ok with having that code in fspath, if it comes with any code that will actually use it (even instead of str()).

[ghost]

A possible solution would be using pathlib2 in Python 3.5 instead of stdlib pathlib. We should, however, be careful since some patches in PathInfo done for Python 2 might actually address pathlib/pathlib2 differences.

How will that work with third parties, @Suor ? for example, using tmp_path in Python 3.5

[ghost]

str doesn't return relpath as in our PathInfo(it is done for a reason though)

@efiop , could you remind me the reason for overwriting __str__ ?

[efiop]

@mroutis To show relpath, when using in messages.

[ghost]

@Suor pointed out that pytest uses pathlib2 for < 3.6 https://github.com/pytest-dev/pytest/blob/8cdf9d43a9f277782310bedd7431fad20474085f/setup.py#L11

[ghost]

_pytest/compat.py: https://github.com/pytest-dev/pytest/blob/8cdf9d43a9f277782310bedd7431fad20474085f/src/_pytest/compat.py#L60-L71

[ghost]

I love how they are already using typing :heart:
https://github.com/pytest-dev/pytest/commit/562d4811d59e495bdfd3123a7f725d55462769ec#diff-4ec37b1ad5b7d41fdac56c907d08dde1