Use pathlib instead of os.path module #641
Conversation
Secrus
force-pushed
the
use-pathlib
branch
2 times, most recently
from
18d0424
to
86ccc11
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Secrus
force-pushed
the
use-pathlib
branch
2 times, most recently
from
10bc681
to
73ae2ee
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| @@ -147,11 +146,11 @@ def test_temporary_directory_python_3_9_or_older(mocker: MockerFixture) -> None: | |||
| mocked_temp_dir = mocker.patch("tempfile.TemporaryDirectory") | |||
| mocked_mkdtemp = mocker.patch("tempfile.mkdtemp") | |||
|
|
|||
| mocked_mkdtemp.return_value = "hello from test" | |||
| mocked_mkdtemp.return_value = Path("hello from test") | |||
There was a problem hiding this comment.
That does not make sense, since we are mocking tempfile.mkdtemp.
There was a problem hiding this comment.
Do you have any better idea? I am tempted to just noqa this and leave it as it was...
There was a problem hiding this comment.
Does it result in a warning? I wonder how ruff would decide that this string should be Path?
| @@ -135,7 +134,7 @@ def test_temporary_directory_python_3_10_or_newer(mocker: MockerFixture) -> None | |||
|
|
|||
| mocker.patch.object(sys, "version_info", (3, 10)) | |||
| with temporary_directory() as tmp: | |||
| assert tmp | |||
| assert str(tmp) | |||
There was a problem hiding this comment.
This assert has always been quite weak. Now it does not make any sense since the string representation of a Path will always be truthy as far as I know. We should probably do it similar to the test some lines below and set the return value of mocked_temp_dir. Then, we can check for the correct path.
radoering
force-pushed
the
use-pathlib
branch
from
73ae2ee
to
9bbffab
Compare
|
| """Like os.path.splitext, but take off .tar too""" | ||
| base, ext = posixpath.splitext(path) | ||
| base, ext = path.stem, path.suffix |
There was a problem hiding this comment.
That's not the same:
>>> posixpath.splitext('foo/bar.baz')
('foo/bar', '.baz')
>>> path = Path('foo/bar.baz')
>>> path.stem, path.suffix
('bar', '.baz')
However, it does not matter for our uses so it might still be ok (if we fix the docstring).
On the other side, we could change the signature to allow str | Path and keep the old implementation. We don't even need a noqa as it seems. I slightly tend to this variant. Then, we don't have to convert the str to Path in link.py.
| @@ -133,19 +132,19 @@ def is_python_project(path: Path) -> bool: | |||
| return pyproject or setuptools_project | |||
|
|
|||
|
|
|||
| def is_archive_file(name: str) -> bool: | |||
| def is_archive_file(name: Path) -> bool: | |||
There was a problem hiding this comment.
| def is_archive_file(name: Path) -> bool: | |
| def is_archive_file(name: str | Path) -> bool: |
From an interface's point of view, I think it makes sense to allow plain file names.
Migrated from
os.pathtopathlibusage.