-
Notifications
You must be signed in to change notification settings - Fork 43
adds copy-constructor to UPath #52
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
To further motivate this: Copying is also the behavior of the def do_stuff(path: Union[str, os.PathLike, Path]) -> None:
path = Path(path)
assert isinstance(path, Path)
... When def do_stuff(path: Union[str, os.PathLike, Path, UPath]) -> None:
path = make_upath(path)
assert isinstance(path, UPath)
...
def make_upath(maybe_path: Union[str, PathLike, Path, UPath]) -> UPath:
return maybe_path if isinstance(maybe_path, UPath) else UPath(maybe_path) |
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.
That's very useful IMO 👍 I added a few comments:
upath/core.py
Outdated
return cls.__new__( | ||
cls, | ||
*new_args, | ||
**new_kwargs, | ||
) |
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.
You might also rewrite args
and kwargs
and simply continue with the constructor, but also fine like this.
upath/core.py
Outdated
@@ -123,6 +123,24 @@ class UPath(pathlib.Path, PureUPath, metaclass=UPathMeta): | |||
_default_accessor = _FSSpecAccessor | |||
|
|||
def __new__(cls, *args, **kwargs): | |||
if len(args) == 1 and isinstance(args[0], cls): |
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.
Shouldn't this also check for empty kwargs
?
if len(args) == 1 and isinstance(args[0], cls): | |
if len(args) == 1 and isinstance(args[0], cls) and len(kwargs) == 0: |
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.
What if someone wants to do something like:
a = UPath("s3://bucket/folder", key="...", secret="...")
b = UPath(a, 'file')
I think checking if len(args) = 1
would make this not possible. Could we instead pop the first arg if its an instance of UPath and append the rest of args to new_args? I would also say allow kwargs to be set and do something like new_kwargs.update(kwargs)
.
upath/core.py
Outdated
new_args = ( | ||
other._format_parsed_parts( | ||
other._drv, other._root, other._parts | ||
), | ||
) | ||
new_kwargs = {} | ||
if hasattr(other, "_kwargs"): | ||
new_kwargs = other._kwargs.copy() | ||
new_kwargs.pop("_url", None) |
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.
You could directly use __reduce__
instead:
new_args = ( | |
other._format_parsed_parts( | |
other._drv, other._root, other._parts | |
), | |
) | |
new_kwargs = {} | |
if hasattr(other, "_kwargs"): | |
new_kwargs = other._kwargs.copy() | |
new_kwargs.pop("_url", None) | |
_, new_args, new_kwargs = other.__reduce__() |
This looks good to me. I'll go ahead and merge it. Thanks @normanrz! |
Adds a new capability to the
UPath
constructor for copyingUPath
objects including their attached kwargs.Fixes #49