-
-
Notifications
You must be signed in to change notification settings - Fork 369
CQ: Fix Ruff PTH110: os.path.exists()
should be replaced by Path.exists()
#6524
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
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.
I made some suggestions using the GitHub Web-UI. So suggested changes are checked by pre-commit...
Most changes replace more os.path usage, to avoid we have to touch those lines again alter. If you feel that is out of scope or interferes with your plans / stategy for introducing more pathlib, please feel free to reject suggested changes.
One thing I noticed in this context is that there is a number of cases where Path(file_path).exists()
is preceeded by os.path.join()
. I guess once we replace os.path.join
by pathlib usage as well, we would have to change the lines again that we modified here. Not sure if ruff would catch that and fix automatically...
So I am wondering if there is an order in which the introduction of pathlib would be most efficient....
Since the amount of changes were really big, fixing all the suggestions for pathlib that are now available (they are better and better as time goes) would be too much (over like 2000 places), I reverted back and stuck to only fixing one single rule. I didn't want to pass my time analyzing if the output is accepted as a Path object vs a string, and the effect on the functions called/calling with that new value. For this one, it is a boolean, and maybe it was converted to a Path object for nothing, but I didn't want to analyze too close. So, if you feel like adding more to the scope, I don't really mind, but it is an extra risk for a PR on the bigger side. |
Oh, seems like there's a place that we will need to revert for a test on macOS |
I see. Fully understand. |
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.
Found two things to revert
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: Stefan Blumentrath <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…tead of checking if exists or not
Ready to review again, checks passing now. And others: sorry for the emails, the new files tab in the web UI doesn't implement batching multiple suggestions |
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.
There is one file where I earlier overlooked that a path was already a pathlib object. Not sure how important that is in a test...
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
python/grass/tools/tests/grass_tools_session_tools_pack_test.py
Outdated
Show resolved
Hide resolved
…rgument as a pathlib.Path in python/grass/tools/tests/grass_tools_session_tools_pack_test.py
…110-os-path-exists
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. Not being a software developer by education, I do see the value of annotations even clearer...
Ruff rule: https://docs.astral.sh/ruff/rules/os-path-exists/
Ignored the "
python/grass/__init__.py
" file, as it might be the most critical if there's a chance that there is a slight performance penalty to this higher level function.