Skip to content

Change subclassing behavior to Python standard #56

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 9 commits into from
Apr 12, 2022

Conversation

normanrz
Copy link
Collaborator

@normanrz normanrz commented Apr 4, 2022

The current implementation makes it very hard to discern objects of pathlib.Path from objects of UPath (and its subclasses).

upath = UPath("gcs://bucket")
path = pathlib.Path("/tmp")

assert isinstance(upath, UPath)  # true, which is expected
assert isinstance(upath, pathlib.Path)  # also true, which is expected, because UPath is a subclass of pathlib.Path

assert isinstance(path, pathlib.Path)  # true, which is expected
assert isinstance(path, UPath) # also true, but NOT expected

This is due to the fact that UPath overwrites isinstance and issubclass checks in a meta class.

In my user code, I currently discern between the two based on the existence of the _kwargs attribute.

if hasattr(path, "_kwargs"):
    upath = cast(UPath, path)
    ...

I think the current behavior is surprising and not very pythonic.

In this PR, I remove the multi-subclassing of UPath. This was unneccassary, because _flavour can be implemented directly in UPath and pathlib.Path is a subclass of pathlib.PurePath itself. With that, I could also remove the meta classes.
This PR also makes upath play more nicely with type checkers such as mypy or pylance.

@normanrz normanrz self-assigned this Apr 6, 2022
Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think valid isinstance checks are quite helpful here! Returning the parent class in new might still lead to confusion, but I think the current handling is more appropriate (see also the comment). Besides that I just have one minor nitpick:

Copy link
Contributor

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: I assume that if issubclass(cls, UPath): in __new__ must always be True, no?

@normanrz
Copy link
Collaborator Author

normanrz commented Apr 6, 2022

PS: I assume that if issubclass(cls, UPath): in __new__ must always be True, no?

Theoretically, you can do UPath.__new__(pathlib.Path, ".") as in https://github.com/fsspec/universal_pathlib/blob/main/upath/tests/test_core.py#L97. But I don't think that is something that should be done.

@normanrz
Copy link
Collaborator Author

@andrewfulton9 Are you ok with merging this? It kinda reverts #29, but I think it provides a more standard-compliant behavior.

@andrewfulton9
Copy link
Collaborator

@normanrz, yeah, this looks good to me. I'll go ahead and merge and close it out.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants