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

bpo-31163: Adding return values to pathlib's rename and replace methods #4055

Closed
wants to merge 2 commits into from

Conversation

@ghost
Copy link

commented Oct 20, 2017

Hello,

this is a patch for bpo-31163. I am not an author, but I noticed the same thing and found the open issue (didn't know how to respond to it on bug tracker tho).
As I didn't see anyone opposing the change, I assumed it is OK, and tried to implement it.

This is my first attempt to contribute to Python, so please tell me if I missed something.

Regarding the code - not sure what the conventions in the standard library are, there were 3 options:

  1. target = PurePath(target)
  2. target = self.class(target) - I chose this one
  3. target = type(self)(target)

If I should've picked other option, please do correct me

Thanks!

https://bugs.python.org/issue31163

@the-knights-who-say-ni

This comment has been minimized.

Copy link

commented Oct 20, 2017

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@@ -922,6 +922,9 @@ call fails (for example because the path doesn't exist):
*target* exists and is a file, it will be replaced silently if the user
has permission. *target* can be either a string or another path object::

If the provided *target* is a path object, it is the method's return value.
Otherwise (if it is a string), new path object is created from it and returned.

This comment has been minimized.

Copy link
@Syeberman

Syeberman Oct 20, 2017

The example below needs to be updated now that p.rename(target) is returning a value (double-check this in a terminal):

>>> p = Path('foo')
>>> p.open('w').write('some text')
9
>>> target = Path('bar')
>>> p.rename(target)
Path('bar')
>>> target.open().read()
'some text'
"""
if self._closed:
self._raise_closed()
self._accessor.rename(self, target)
if isinstance(target, str):
target = self.__class__(target)

This comment has been minimized.

Copy link
@Syeberman

Syeberman Oct 20, 2017

It might be safer to call self.__class__ unconditionally (i.e. if a new type of object besides PurePath or str is added to pathlib in the future). If we want to support original object return in pathlib, that's really something that should be added to PurePath._from_parts.

if self._closed:
    self._raise_closed()
self._accessor.rename(self, target)
return self.__class__(target)

This comment has been minimized.

Copy link
@ghost

ghost Oct 20, 2017

Author

Yes, I was considering this approach as well, just wanted to avoid having two objects representing the same thing.
I have no strong preferences for either approach

@csabella

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

The user account that created this pull request has been deleted, so I'm going to close this.

@csabella csabella closed this May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.