GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-84538: add strict argument to pathlib.PurePath.relative_to by domragusa · Pull Request #19813 · python/cpython Conversation
|
|
Thanks! I expected to be able to use Python 3.9 included an |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
A what's new entry should go in What's New 3.11. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Terry Jan Reedy <[email protected]>
|
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: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| .. versionadded:: 3.11 | ||
| The *strict* argument (pre-3.11 behavior is strict). | ||
| .. versionadded:: 3.10 | ||
| The *strict* argument (pre-3.10 behavior is strict). |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Duplication?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't notice this, thanks.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| ValueError: '/etc/passwd' is not on the same drive as 'foo' OR one path is relative and the other is absolute. | ||
|
|
||
| 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. |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This is indeed tricky to phrase! How about: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| .. versionadded:: 3.11 | ||
| The *strict* argument (pre-3.11 behavior is strict). | ||
| .. versionadded:: 3.10 | ||
| The *strict* argument (pre-3.10 behavior is strict). |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't notice this, thanks.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 Perhaps something like |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Yeah, strict doesn't give any clue about the actual function. I like walk_up, I'll change it to that tomorrow. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Sorry for the long delay, I just updated the code to follow the suggestion you made about the name of the option. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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. |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @brettcannon: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@domragusa: Status check is done, and it's a failure or timed out ❌. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm not sure I understand the last comment, what status check has failed? Should I do anything else? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Status check is done, and it's a success ✅. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@domragusa thanks so much for your patience with this! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm very happy to see this being merged, thanks to @brettcannon and everyone else who helped along the way :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ok, the table at the end of the pathlib doc was augmented, and relpath was listed with a comment. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
FYI, I've opened a bug and PR to address one final incompatibility between |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.