Skip to content

gh-84538: add strict argument to pathlib.PurePath.relative_to #19813

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

Merged
merged 17 commits into from
Oct 28, 2022

Conversation

domragusa
Copy link
Contributor

@domragusa domragusa commented Apr 30, 2020

By default, :meth: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 change adds a walk_up parameter that can be set to allow for using .. to calculate the relative path.

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

Automerge-Triggered-By: GH:brettcannon

@domragusa domragusa changed the title bpo-40358: add strict param to pathlib.PurePath.relative_to bpo-40358: add strict argument to pathlib.PurePath.relative_to May 28, 2020
@zeehio
Copy link

zeehio commented Oct 30, 2020

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?

@domragusa
Copy link
Contributor Author

domragusa commented Oct 30, 2020 via email

@terryjreedy
Copy link
Member

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

@domragusa
Copy link
Contributor Author

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`.)

Copy link
Contributor

@barneygale barneygale left a comment

Choose a reason for hiding this comment

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

Looks good - a couple of comments on the docs.

Comment on lines 568 to 571
.. versionadded:: 3.11
The *strict* argument (pre-3.11 behavior is strict).
.. versionadded:: 3.10
The *strict* argument (pre-3.10 behavior is strict).
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't notice this, thanks.


NOTE: This function is part of :class:`PurePath` and works with strings. It does not check or access the underlying file structure.
If the path doesn't start with *other* and *strict* is ``True``, :exc:`ValueError` is raised. If *strict* is ``False`` and one path is relative and the other is absolute or if they reference different drives :exc:`ValueError` is raised.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the key difference should be spelled out in terms of whether adding .. entries is allowed. How about something like this?

In strict mode (the default), the path must start with *other*. In non-strict 
mode, ``..`` entries may be added to form the relative path. In all other 
cases, such as the paths referencing different drives, :exe:`ValueError` is 
raised.

.. warning::
    Non-strict mode assumes that no symlinks are present in the path; you 
    should call :meth:`~Path.resolve` first to ensure this.

@barneygale
Copy link
Contributor

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`.)

Copy link
Contributor Author

@domragusa domragusa left a comment

Choose a reason for hiding this comment

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

@barneygale I've added the what's new entry with the clearer language and fixed the documentation as you suggested. Thanks.

Comment on lines 568 to 571
.. versionadded:: 3.11
The *strict* argument (pre-3.11 behavior is strict).
.. versionadded:: 3.10
The *strict* argument (pre-3.10 behavior is strict).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't notice this, thanks.

@barneygale
Copy link
Contributor

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?

@domragusa
Copy link
Contributor Author

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

@domragusa
Copy link
Contributor Author

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

Comment on lines 566 to 569
When *walk_up* is False, the default, the path must start with *other*,
when it's True ``..`` entries may be added to form the relative path. In
all other cases, such as the paths referencing different drives,
:exc:`ValueError` is raised.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this documentation needs to occur before the examples of using walk_up.

So, either move it after "ValueError is raised", or split the examples into two parts.

@barneygale
Copy link
Contributor

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.

@CAM-Gerlach
Copy link
Member

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?

@domragusa
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@miss-islington
Copy link
Contributor

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

@brettcannon brettcannon changed the title bpo-40358: add strict argument to pathlib.PurePath.relative_to gh-84538: add strict argument to pathlib.PurePath.relative_to Oct 28, 2022
@domragusa
Copy link
Contributor Author

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

@miss-islington
Copy link
Contributor

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

@miss-islington miss-islington merged commit e089f23 into python:main Oct 28, 2022
@brettcannon
Copy link
Member

@domragusa thanks so much for your patience with this!

@domragusa
Copy link
Contributor Author

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

@ctismer
Copy link
Contributor

ctismer commented Oct 29, 2022

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?

@domragusa
Copy link
Contributor Author

domragusa commented Oct 29, 2022

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.

@ctismer
Copy link
Contributor

ctismer commented Oct 31, 2022

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 :)

@zeehio
Copy link

zeehio commented Oct 31, 2022

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

@barneygale
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.