-
Notifications
You must be signed in to change notification settings - Fork 14
fixes issue with UPath 0.3.x #1374
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
Still working on the coverage reports, Windows tests and Changelog, but the rest is already reviewable. |
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
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.
Cool stuff, thanks for pushing this through!
I added a couple of comments, mostly concerning consistency in the type hints. Not all of those things were introduced in this PR, but I noticed them now while reading the respective code.
One thing I encountered a couple of times: I assumed that functions should either support taking UPath
only or str | PathLike | UPath
. However, I also found other combinations, like PathLike | UPath
but not string, and I also found other orderings like PathLike | UPath | str
. Maybe this could be unified. Of course, if there is a specific reason for the differences, feel free to explain it.
Also, I’m not sure about the changes in ci.yml. Were they necessary because you updated other dependencies? Could you explain the changes?
Description:
UPath
is no longer a subclass ofPath
. This PR removesPath
almost entirely in favor ofUPath
inside our codebase. Users can still pass inPath
s and the examples also still have paths.Todos:
Make sure to delete unnecessary points or to check all before merging: