cpython icon indicating copy to clipboard operation
cpython copied to clipboard

bpo-40358: add strict argument to pathlib.PurePath.relative_to

Open domragusa opened this issue 5 years ago • 11 comments

By default pathlib.PurePath.relative_to doesn't deal with paths that are not a direct prefix of the other raising an exception in that instance, this patch adds the parameter strict that can be set to False to make the function more flexible.

example:

>>> p = PurePosixPath('/etc/passwd')
>>> p.relative_to('/etc')
PurePosixPath('passwd')
>>> p.relative_to('/usr')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "pathlib.py", line 940, in relative_to
    raise ValueError(error_message.format(str(self), str(formatted)))
ValueError: '/etc/passwd' does not start with '/usr'
>>> p.relative_to('/usr', strict=False)
PurePosixPath('../etc/passwd')

https://bugs.python.org/issue40358

domragusa avatar Apr 30 '20 10:04 domragusa

Thanks! I expected to be able to use relative_to() as described in this PR, I'm happy to know it is being worked on.

Python 3.9 included an is_relative_to() method to PurePath(). Should that method gain a strict argument as well?

zeehio avatar Oct 30 '20 06:10 zeehio

I don't know, a is_relative_to with that option would return always true because when you're allowed to traverse the tree structure every path is relative to the other (only exception being when one path is absolute and the other is not or if they are on different discs on windows) anyway if there are useful use cases that I'm missing let me know, I have no problem adding that functionality.

domragusa avatar Oct 30 '20 13:10 domragusa

A what's new entry should go in What's New 3.11.

terryjreedy avatar May 14 '21 22:05 terryjreedy

Hi, I'm sorry but I'm not sure about the what's new entry, should I put it under Improved Modules > pathlib? Something like:

pathlib
-------

:meth:`pathlib.PurePath.relative_to` now allows a path that is not a direct
child of the current if the *strict* keyword-only argument is set to False,
the new behavior is more consistent with :meth:`os.relpath`.
(Contributed by Domenico Ragusa in :issue:`40358`.)

domragusa avatar May 15 '21 10:05 domragusa

Hi, I'm sorry but I'm not sure about the what's new entry, should I put it under Improved Modules > pathlib? Something like:

pathlib
-------

:meth:`pathlib.PurePath.relative_to` now allows a path that is not a direct
child of the current if the *strict* keyword-only argument is set to False,
the new behavior is more consistent with :meth:`os.relpath`.
(Contributed by Domenico Ragusa in :issue:`40358`.)

This is indeed tricky to phrase! How about:

:meth:`pathlib.PurePath.relative_to` now supports generating relative paths
containing ``..`` entries if its new *strict* keyword-only argument is set to
``False``. The new behavior is more consistent with :func:`os.path.relpath`.
(Contributed by Domenico Ragusa in :issue:`40358`.)

barneygale avatar May 17 '21 00:05 barneygale

I'm trying to think if there's a better word than 'strict'. In other places, 'strict' usually refers to our behaviour when we get an error from the OS. In this case it's more to do with whether we tolerate the possibility of the path's meaning changing in our pure operation that can't ever raise OSError anyway.

Perhaps something like parents=True to enable the new behaviour, mirroring mkdir()? Or walk_up?

barneygale avatar May 17 '21 01:05 barneygale

Yeah, strict doesn't give any clue about the actual function. I like walk_up, I'll change it to that tomorrow.

domragusa avatar May 18 '21 00:05 domragusa

Sorry for the long delay, I just updated the code to follow the suggestion you made about the name of the option.

domragusa avatar May 29 '21 11:05 domragusa

Aside from a small docs issue, this looks great to me. Really useful feature, nice implementation :)

Others may want to chip in on the naming of walk_up.

barneygale avatar Jun 24 '21 10:06 barneygale

Hey @domragusa , there has been further interest in this over on the Python discourse; any chance you could tweak the docs, fix the merge conflicts and retrigger the builds?

CAM-Gerlach avatar Dec 10 '21 21:12 CAM-Gerlach

While moving away from os.path, I encounter this exact issue and wanted to contribute it as well, but I notice that you already did @domragusa thank you very much for that :) Looking forward to see this merged :tada:

cmaureir avatar Oct 14 '22 12:10 cmaureir

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

bedevere-bot avatar Oct 21 '22 22:10 bedevere-bot

I have made the requested changes; please review again

domragusa avatar Oct 22 '22 02:10 domragusa

Thanks for making the requested changes!

@brettcannon: please review the changes made to this pull request.

bedevere-bot avatar Oct 22 '22 02:10 bedevere-bot

@domragusa: Status check is done, and it's a failure or timed out ❌.

miss-islington avatar Oct 28 '22 22:10 miss-islington

I'm not sure I understand the last comment, what status check has failed? Should I do anything else?

domragusa avatar Oct 28 '22 23:10 domragusa

Status check is done, and it's a success ✅.

miss-islington avatar Oct 28 '22 23:10 miss-islington

@domragusa thanks so much for your patience with this!

brettcannon avatar Oct 28 '22 23:10 brettcannon

I'm very happy to see this being merged, thanks to @brettcannon and everyone else who helped along the way :)

domragusa avatar Oct 28 '22 23:10 domragusa

Maybe I‘m misunderstanding, but this was an incompatibility to path.relpath which is now fixed. Should it not be considered a bug, and the fix be back-ported to a few versions?

ctismer avatar Oct 29 '22 12:10 ctismer

I don't think anybody is currently relying on pathlib for this behaviour, in fact when I looked around for anwsers they were generally saying to use os.path.relpath and also the original developer told me it was the intended behaviour (see #84538), so I'm not convinced we should consider it as a bug.

Anyway, if there are specific arguments for backporting it's fine by me.

domragusa avatar Oct 29 '22 17:10 domragusa

Ok, the table at the end of the pathlib doc was augmented, and relpath was listed with a comment. You are right, it is not a bug. Still, we were trying to convert everything, but this is the only function that has no equivalent at os.path, and it would take quite long until this change makes it through our supported versions from 3.7 on :)

ctismer avatar Oct 31 '22 15:10 ctismer

Hi

With respect to the backporting, I took this pull request and created a small package out of it, following the backports package guidelines.

I included some of the tests.

https://pypi.org/project/backports-pathlib-relative-to/0.1.0/

https://github.com/zeehio/backports-pathlib-relative-to

Issues and pull requests are welcome

zeehio avatar Oct 31 '22 20:10 zeehio

FYI, I've opened a bug and PR to address one final incompatibility between PurePath.relative_to() and os.path.relpath().

barneygale avatar Nov 03 '22 17:11 barneygale