-
-
Notifications
You must be signed in to change notification settings - Fork 370
CQ: Fix Ruff PTH113: os.path.isfile() should be replaced by Path.is_file()
#6573
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Path(os.path.join(self.tmp_mapset_path, "signatures", "libsvm")).is_dir() | |
| Path(self.tmp_mapset_path, "signatures", "libsvm").is_dir() |
|
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) |
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.unlinkwithmissing_ok=Trueinstead of checking if it is a file. The only difference, is that usingPath("something").unlink(missing_ok=True)will throw anIsADirectoryError. But in the context of a test, where the file to delete is a class property, I accepted that.