Skip to content
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

Inherit from PathBase instead of Path #193

Open
2 tasks
Tracked by #223
ap-- opened this issue Feb 18, 2024 · 3 comments · May be fixed by #270
Open
2 tasks
Tracked by #223

Inherit from PathBase instead of Path #193

ap-- opened this issue Feb 18, 2024 · 3 comments · May be fixed by #270
Labels
compatibility 🤝 Compatibility with stdlib pathlib enhancement 🚀 New feature or request
Milestone

Comments

@ap--
Copy link
Collaborator

ap-- commented Feb 18, 2024

This will be the next big change in UPath.

In the near future we will derive from pathlib_abc.PathBase:

from pathlib_abc import PathBase

class UPath(PathBase):
    ...

We will have to consider a few things before this is possible:

  • a clean deprecation mechanism for code that checks isinstance(p, Path)
  • how do WindowsUPath and PosixUPath change?
@ap-- ap-- added enhancement 🚀 New feature or request compatibility 🤝 Compatibility with stdlib pathlib labels Feb 18, 2024
@barneygale
Copy link

Here's a potential class hierarchy:

image

You're right that users would need to switch to checking isinstance(p, PathBase) to check for a rich path object.

Personally I see no reason for there to be PosixUPath and WindowsUPath classes. You could provide a LocalUPath class and leave it there, maybe?

@ap--
Copy link
Collaborator Author

ap-- commented May 4, 2024

Hi @barneygale

Thank you for your message! I hope that I can get to this soon, so that it's ready for the Python3.13 release. I just saw that the 3.13 alpha is scheduled to release by the end of the month already. Are there any subtle changes that I should be aware of from versions 3.12 to 3.13?

I currently assume that at some point in time I will be able to do:

if sys.version_info >= (3, XX):
    from pathlib import PathBase  # or maybe pathlib.abc or somewhere else in the stdlib
else:
    from pathlib_abc import PathBase

You're right that users would need to switch to checking isinstance(p, PathBase) to check for a rich path object.

Do you have an idea how I could raise a deprecation warning when user code calls:

p = UPath(...)

isinstance(p, pathlib.Path)  # this should throw a warning
isinstance(p, str)  # (or anything other than pathlib.Path) should not raise a warning

Metaclasses with custom __instancecheck__ don't help because the logic is inverted. And overriding UPath.__class__ to return some object that then does magic to figure out the call context (I have not checked if that would work) seems like a horrible hack...

Personally I see no reason for there to be PosixUPath and WindowsUPath classes. You could provide a LocalUPath class and leave it there, maybe?

Yeah, ideally I would like to avoid having PosixUPath and WindowsUPath. But a while ago a few custom attributes and methods were added to UPath and so I needed to subclass. For relative paths, these are the only option as of now, because in the filesystem_spec world all paths are absolute. At some point in time, I think it would be good to have a LocalUPath class as you suggest, as well as a PureUPath that supports the different parsers (flavours) as well as relative paths to cover those use cases.

Cheers,
Andreas

@barneygale
Copy link

Do you have an idea how I could raise a deprecation warning [...]

I don't think so :(. I think the best you could do is to raise a warning from __fspath__(), which is arguably the most important interface difference between Path and PathBase.

@ap-- ap-- added this to the v0.3.0 milestone Aug 23, 2024
@ap-- ap-- linked a pull request Sep 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compatibility 🤝 Compatibility with stdlib pathlib enhancement 🚀 New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants