Skip to content

Conversation

@echoix
Copy link
Member

@echoix echoix commented Nov 2, 2025

Ruff rule: https://docs.astral.sh/ruff/rules/os-path-isfile/

I've also fixed the other PTH ruff rules in the same lines changed, when appropriate, so there's a little more than only PTH113 fixed.

For tests, I mostly agreed on the simplification of using pathlib.Path.unlink with missing_ok=True instead of checking if it is a file. The only difference, is that using Path("something").unlink(missing_ok=True) will throw an IsADirectoryError. But in the context of a test, where the file to delete is a class property, I accepted that.

In [1]: from pathlib import Path

In [2]: b = Path("test123")

In [3]: b.unlink()
---------------------------------------------------------------------------
IsADirectoryError                         Traceback (most recent call last)
Cell In[3], line 1
----> 1 b.unlink()

File /usr/local/python/3.12.7/lib/python3.12/pathlib.py:1342, in Path.unlink(self, missing_ok)
   1337 """
   1338 Remove this file or link.
   1339 If the path is a directory, use rmdir() instead.
   1340 """
   1341 try:
-> 1342     os.unlink(self)
   1343 except FileNotFoundError:
   1344     if not missing_ok:

IsADirectoryError: [Errno 21] Is a directory: 'test123'

In [4]: 
@echoix echoix requested a review from ninsbl November 2, 2025 20:59
@github-actions github-actions bot added GUI wxGUI related vector Related to vector data processing raster Related to raster data processing temporal Related to temporal data processing Python Related code is in Python libraries module general display imagery tests Related to Test Suite labels Nov 2, 2025
Copy link
Member

@ninsbl ninsbl left a comment

Choose a reason for hiding this comment

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

Nice. Again, I learned something from reviewing (have not seen or used (p := Path(mfile)) before.

Just some minor suggestions where I think os.path.join inside Path initialisation is not needed...

os.path.join(dbase, location, mapset, "WIND")
if (
Path(mapset).is_dir()
and Path(os.path.join(dbase, location, mapset, "WIND")).is_file()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
and Path(os.path.join(dbase, location, mapset, "WIND")).is_file()
and Path(dbase, location, mapset, "WIND").is_file()

# location already exists?
if os.path.isdir(os.path.join(database, location)):
if Path(os.path.join(database, location)).is_dir():
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if Path(os.path.join(database, location)).is_dir():
if Path(database, location).is_dir():
I_make_signatures_dir(I_SIGFILE_TYPE_SIG)
self.assertTrue(
os.path.isdir(os.path.join(self.tmp_mapset_path, "signatures", "sig"))
Path(os.path.join(self.tmp_mapset_path, "signatures", "sig")).is_dir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path(os.path.join(self.tmp_mapset_path, "signatures", "sig")).is_dir()
Path(self.tmp_mapset_path, "signatures", "sig").is_dir()
I_make_signatures_dir(I_SIGFILE_TYPE_SIGSET)
self.assertTrue(
os.path.isdir(os.path.join(self.tmp_mapset_path, "signatures", "sigset"))
Path(os.path.join(self.tmp_mapset_path, "signatures", "sigset")).is_dir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path(os.path.join(self.tmp_mapset_path, "signatures", "sigset")).is_dir()
Path(self.tmp_mapset_path, "signatures", "sigset").is_dir()
I_make_signatures_dir(I_SIGFILE_TYPE_SIG)
self.assertTrue(
os.path.isdir(os.path.join(self.tmp_mapset_path, "signatures", "sig"))
Path(os.path.join(self.tmp_mapset_path, "signatures", "sig")).is_dir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path(os.path.join(self.tmp_mapset_path, "signatures", "sig")).is_dir()
Path(self.tmp_mapset_path, "signatures", "sig").is_dir()
I_make_signatures_dir(I_SIGFILE_TYPE_SIGSET)
self.assertTrue(
os.path.isdir(os.path.join(self.tmp_mapset_path, "signatures", "sigset"))
Path(os.path.join(self.tmp_mapset_path, "signatures", "sigset")).is_dir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path(os.path.join(self.tmp_mapset_path, "signatures", "sigset")).is_dir()
Path(self.tmp_mapset_path, "signatures", "sigset").is_dir()
I_make_signatures_dir(I_SIGFILE_TYPE_LIBSVM)
self.assertTrue(
os.path.isdir(os.path.join(self.tmp_mapset_path, "signatures", "libsvm"))
Path(os.path.join(self.tmp_mapset_path, "signatures", "libsvm")).is_dir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path(os.path.join(self.tmp_mapset_path, "signatures", "libsvm")).is_dir()
Path(self.tmp_mapset_path, "signatures", "libsvm").is_dir()
I_make_signatures_dir(I_SIGFILE_TYPE_LIBSVM)
self.assertTrue(
os.path.isdir(os.path.join(self.tmp_mapset_path, "signatures", "libsvm"))
Path(os.path.join(self.tmp_mapset_path, "signatures", "libsvm")).is_dir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Path(os.path.join(self.tmp_mapset_path, "signatures", "libsvm")).is_dir()
Path(self.tmp_mapset_path, "signatures", "libsvm").is_dir()
@wenzeslaus
Copy link
Member

I had issues recently with Path.exists that it was behaving differently than os.path equivalent when permissions were read-only. At least one Linux, calling Path.exists() raised permission denied error, but os.path did not where parent directory was read-only for a file which does not exist. I found that when testing pre-conditions in an r.pack test. That's just to say that Path or os.path may not be 100% the same, although I assume that's the intention and the general direction where the library is moving, so I don't know whether to take any action here.

@pytest.mark.skipif(sys.platform.startswith("win"), reason="Dir still writable")
def test_output_dir_is_not_writable(xy_raster_dataset_session_for_module, tmp_path):
    """Check behavior when directory is not writable"""
    tools = Tools(session=xy_raster_dataset_session_for_module)
    parent = tmp_path / "parent_dir"
    parent.mkdir()
    # This should work for Windows according to the doc, but it does not.
    parent.chmod(stat.S_IREAD)
    output = parent / "output_1.rpack"
    # Calling output.exists() gives permission denied on Linux, but os.path does not.
    assert not os.path.exists(output)  # noqa: PTH110
    assert output.parent.exists()
    assert output.parent.is_dir()
    with pytest.raises(
        CalledModuleError,
        match=rf"(?s)'{re.escape(str(output.parent))}'.*is not.*writable",
    ):
        tools.r_pack(input="rows_raster", output=output)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

display general GUI wxGUI related imagery libraries module Python Related code is in Python raster Related to raster data processing temporal Related to temporal data processing tests Related to Test Suite vector Related to vector data processing

3 participants