-
Notifications
You must be signed in to change notification settings - Fork 224
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
Migrate from os.path to pathlib #3119
Conversation
Will merge the PR in 24 hours if no objections. |
Do you want to enable ruff's PTH rules in this PR as mentioned at #1834 (comment), or in a separate PR? |
Yes, but enabling PTH rules result in many violations against the PTH123 rule. Better to do it in a separate PR and also need to discuss if we want to replace |
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'm seeing that a lot of the assert os.path.exists()
lines have been removed, but not replaced with Path.exists()
, possibly because there is a Path.unlink()
directly afterwards that would raise a FileNotFoundError
if the file was missing (see https://docs.python.org/3/library/pathlib.html#pathlib.Path.unlink), is that correct?
I think there are some cases where we do need to explicitly do assert Path.exists()
, especially if the test name is something like test_file_exists
. Otherwise it's not apparent that .unlink()
actually implicitly checks if the file exists already.
We could also apply |
Added back in ee0ba18.
In some cases migrating from |
Migrate from
os.path
topathlib.Path
following this table: https://docs.python.org/3/library/pathlib.html#correspondence-to-tools-in-the-os-module.Please note that:
is equivalent to
Path(fname).unlink(missing_ok=True)
.is equivalent to
Path(fname).unlink(missing_ok=False)
or justPath(fname).unlink()
.Closes #1834.