Avoid using py.path
#207
Avoid using py.path
#207
Conversation
Both `tmpdir` and `tmp_path` fixtures create temporary directories, but the legacy `tmpdir` is based on third-party `py.path.local` objects whereas `tmp_path` is based on the standard library `pathlib.Path`.
|
There are three failures in one of the CI jobs, but the same failures are also present in |
ConorMacBride
left a comment
ConorMacBride
left a comment
There was a problem hiding this comment.
Thanks @eerovaher, this is great! Just one suggestion, and then hopefully we can get it merged soon so packages can disable the legacy path.
I was thinking of how we could disable legacypath in the pytest-mpl tests based on the pytest version installed, but I can't think of a simple way to do that.
pytest_mpl/plugin.py
Outdated
| if Version(pytest.__version__) < Version("7.0.0"): | ||
| path = Path(item.fspath) | ||
| else: | ||
| path = item.path |
There was a problem hiding this comment.
Do you think this would be sufficient?
| if Version(pytest.__version__) < Version("7.0.0"): | |
| path = Path(item.fspath) | |
| else: | |
| path = item.path | |
| path = getattr(item, "path", Path(item.fspath)) |
Just trying to avoid parsing and comparing the versions. If you think checking the version is better can you do the comparison in a constant at the top of the file.
There was a problem hiding this comment.
This helper function is only needed because older versions of pytest are too different from the recent ones, and having the version check made that explicit. But conveying that in a comment makes the code shorter and removes the need to import Version, so I have adopted your suggestion.
There was a problem hiding this comment.
This simple getattr() call does not work. Sadly I realized this only after this pull request was already merged, so I had to open a follow-up pull request: #213.
`py.path` provides classes for representing filesystem paths, but became obsolete when `pathlib` was added to Python standard library. `pytest` recommends creating temporary directories with the `tmp_path` fixture, which uses `pathlib`, instead of the older `tmpdir` fixture, which uses `py.path`. Furthermore, it is suggested to call `pytest` with `-p no:legacypath` to remove support for `py.path` entirely, which helps ensure `tmpdir` is not used at all. However, this also breaks any code accessing `_pytest.nodes.Node.fspath`. Because `pytest-mpl` includes such code then packages using it cannot turn off `py.path` support to guard against `tmpdir` usage. Although replacing accessing `fspath` in older versions of `pytest` is complicated, it is very simple since `pytest` 7, so now at least the packages using recent versions of `pytest` can choose to make use of the `-p no:legacypath` option.
|
Thanks @eerovaher! 🚀 |
|
Thanks indeed! @ConorMacBride Would it be possible to get a patch release of |
|
@matthewfeickert Of course! We are well due a new release! There has been quite a few new features added so the next release will need to be |
|
Sorry for the delay, |
|
Thanks @ConorMacBride! I appreciate it, as you've now made the
|
3 participants
py.pathprovides classes for representing filesystem paths, but became obsolete whenpathlibwas added to Python standard library.pytestprovides two fixtures for creating temporary directories:tmp_path, which usespathlib, andtmpdir, which usespy.path. The recommendation is to prefertmp_pathovertmpdir, which is what the first commit here does.Furthermore,
pytestsuggests callingpytestwith-p no:legacypathto remove support forpy.pathentirely, which helps ensuretmpdiris not used at all. However, this also breaks any code accessing_pytest.nodes.Node.fspath. Becausepytest-mplaccesses that then packages using it cannot turn offpy.pathsupport to guard againsttmpdirusage. Although replacing accessingfspathin older versions ofpytestis complicated (neitherreportinfo()[0]norlocation[0]work), it is very simple sincepytest7, so the second commit here allows at least the packages using recent versions ofpytestto make use of the-p no:legacypathoption.