Navigation Menu

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

pathlib's relative_to should behave like os.path.relpath #84538

Closed
domragusa mannequin opened this issue Apr 22, 2020 · 17 comments
Closed

pathlib's relative_to should behave like os.path.relpath #84538

domragusa mannequin opened this issue Apr 22, 2020 · 17 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir topic-pathlib type-feature A feature request or enhancement

Comments

@domragusa
Copy link
Mannequin

domragusa mannequin commented Apr 22, 2020

BPO 40358
Nosy @pitrou, @eryksun, @ammaraskar, @CuriousLearner, @CAM-Gerlach, @barneygale, @domragusa, @bowiechen
PRs
  • bpo-40358: add strict parameter to pathlib.PurePath.relative_to #19807
  • gh-84538: add strict argument to pathlib.PurePath.relative_to #19813
  • Files
  • relative_to.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2020-04-22.00:03:03.148>
    labels = ['type-feature', 'library', '3.11']
    title = "pathlib's relative_to should behave like os.path.relpath"
    updated_at = <Date 2022-02-22.23:57:02.772>
    user = 'https://github.com/domragusa'

    bugs.python.org fields:

    activity = <Date 2022-02-22.23:57:02.772>
    actor = 'terry.reedy'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2020-04-22.00:03:03.148>
    creator = 'd.ragusa'
    dependencies = []
    files = ['49095']
    hgrepos = []
    issue_num = 40358
    keywords = ['patch']
    message_count = 11.0
    messages = ['366958', '367037', '367041', '367047', '367490', '367697', '367723', '370254', '393696', '393777', '408249']
    nosy_count = 10.0
    nosy_names = ['pitrou', 'python-dev', 'eryksun', 'ammar2', 'Socob', 'CuriousLearner', 'CAM-Gerlach', 'barneygale', 'd.ragusa', 'bowiechen']
    pr_nums = ['19807', '19813']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue40358'
    versions = ['Python 3.11']

    @domragusa
    Copy link
    Mannequin Author

    domragusa mannequin commented Apr 22, 2020

    Can we improve pathlib.relative_to(other) so that it handles the case of a path not being a direct child of other, like os.path.relpath?

    For example:
    Path('/some/thing').relative_to('/foo') -> Path('../some/thing')

    At the moment it just raises an exception.

    @domragusa domragusa mannequin added 3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Apr 22, 2020
    @pitrou
    Copy link
    Member

    pitrou commented Apr 22, 2020

    The current behaviour is by design. I would not mind adding an option to control it, though.

    If you are new to Python development and want to submit a patch or PR, I recommend reading the Developer's Guide:
    https://devguide.python.org/

    @domragusa
    Copy link
    Mannequin Author

    domragusa mannequin commented Apr 22, 2020

    Thanks for your answer. Yeah, I'm new, I'm reading the guide, sorry for any
    faux pas :)

    Ok, an option would be great as well, a simple True/False switch? Any
    suggestion for the name?
    I'll get back with a proper patch this time.

    On Wed, Apr 22, 2020 at 8:18 PM Antoine Pitrou <report@bugs.python.org>
    wrote:

    Antoine Pitrou <solipsis@pitrou.net> added the comment:

    The current behaviour is by design. I would not mind adding an option to
    control it, though.

    If you are new to Python development and want to submit a patch or PR, I
    recommend reading the Developer's Guide:
    https://devguide.python.org/

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue40358\>


    @eryksun
    Copy link
    Contributor

    eryksun commented Apr 22, 2020

    Note that the implementation of relpath is pure and thus assumes it's working with existing, resolved paths (i.e. "the filesystem is not accessed to confirm the existence or nature of path or start"). For example:

        >>> os.path.relpath('/some/thing', '/symlink')
        '../some/thing'

    If "symlink" targets "/spam/eggs/foo", then the resolved path would be "/spam/eggs/some/thing" instead of "/some/thing". Maybe the relative_to method should default to a strict mode that raises ValueError on ambiguous cases that depend on the "existence or nature" of the paths. I think the current implementation is strict.

    @domragusa
    Copy link
    Mannequin Author

    domragusa mannequin commented Apr 28, 2020

    Yeah, you're right, there's no access to the filesystem and the result
    is generated assuming the paths are already resolved.
    strict seems to be an appropriate name for the option, thanks.

    I've looked into the test suite, it helped a lot especially with
    Windows paths, they were more complicated than I though.
    I've duplicated the tests to verify that it still function as before
    and I've added some tests for values that would raise an exception. It
    works.
    I'm not overly fond of the way I check for unrelated paths, but I
    couldn't think of something more elegant.

    Any input is appreciated.

    @ammaraskar
    Copy link
    Member

    Thank you for your work on this Domenico. For reviewing the code, would you mind creating a Github pull request for it as described here https://devguide.python.org/pullrequest/

    @domragusa
    Copy link
    Mannequin Author

    domragusa mannequin commented Apr 30, 2020

    I may have forgotten to use the proper format for the title of each
    commit, should I delete the pull request and make a new one or can it
    be fixed when (or if) it's pulled?

    On Thu, Apr 30, 2020 at 2:03 AM Roundup Robot <report@bugs.python.org> wrote:

    Change by Roundup Robot <devnull@psf.upfronthosting.co.za>:

    ----------
    nosy: +python-dev
    nosy_count: 5.0 -> 6.0
    pull_requests: +19128
    stage: -> patch review
    pull_request: #19807


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue40358\>


    @domragusa
    Copy link
    Mannequin Author

    domragusa mannequin commented May 28, 2020

    I've solved the conflicts with #63810 (bpo-23082: Better error message for PurePath.relative_to() from pathlib) that was merged in the mean time and improved the documentation.

    Everything appears to be in order, can you take a look at it?

    @domragusa domragusa mannequin added 3.10 only security fixes and removed 3.9 only security fixes labels May 28, 2020
    @terryjreedy
    Copy link
    Member

    On bpo-44078 (closed as duplicate), Mark Hammond made a similar request.

    @terryjreedy terryjreedy added 3.11 only security fixes and removed 3.10 only security fixes labels May 14, 2021
    @barneygale
    Copy link
    Mannequin

    barneygale mannequin commented May 17, 2021

    Also requested in bpo-42234.

    @CAM-Gerlach
    Copy link
    Member

    For the record, requested on Discourse as well, with a fairly similar proposal.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @barneygale
    Copy link
    Contributor

    barneygale commented Jun 26, 2022

    os.path.relpath() always calls abspath(), which is "impure" by pathlib standards.

    If both arguments to relpath() are relative paths then the abspath() calls can probably be removed. It would save a system call, and allow PurePath.relative_to() to call into os.path.relpath() safely:

        def relative_to(self, *other):
            if not other:
                raise TypeError("need at least one argument")
            other = type(self)(*other)
            if self.is_absolute() != other.is_absolute():
                raise ValueError("one path is relative and the other is absolute: "
                                 "{!r}, {!r}".format(str(self), str(other)))
            rel = self._flavour.relpath(self, other)
            return type(self)(rel)

    @barneygale
    Copy link
    Contributor

    An interesting comment in PurePath.relative_to():

            # For the purpose of this method, drive and root are considered
            # separate parts, i.e.:
            #   Path('c:/').relative_to('c:')  gives Path('/')
            #   Path('c:/').relative_to('/')   raise ValueError

    This isn't true for ntpath.relpath():

    >>> import ntpath
    >>> ntpath.relpath('c:\\', 'c:')
    '.'

    Pathlib's behaviour is deliberate but questionable. If we're willing to drop it, then we can call through to os.path.relpath() when users call relative_to(..., strict=False).

    Also, path.is_relative_to(other) would simplify down to other == path or other in path.parents, which is so simple it barely qualifies for a (misleadingly named) method. It might make sense to deprecate it.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 12, 2022

    "C:" is the current directory in drive C, so I think pathlib's answer is the correct one here (of course, I wrote it so I may be biaised :-)).

    @barneygale
    Copy link
    Contributor

    barneygale commented Jul 12, 2022

    Thanks Antoine! That makes sense, but it still seems a odd to me that naked Windows drive paths are the only place where absolute and relative paths can be mixed! Extending your logic:

    1. Should PureWindowsPath('c:/').relative_to('c:foo') also return '/'?
    2. Should PurePosixPath('/').relative_to('foo') also return '/'?

    Both of these raise ValueError at the moment. Maybe this is a "practicality beats purity" thing?

    @pitrou
    Copy link
    Member

    pitrou commented Jul 12, 2022

    I would say it is a practically beats purity thing indeed. I don't remember the details of why I chose to allow it at the time.

    @barneygale
    Copy link
    Contributor

    Correction to my earlier post:

    C: is a relative path, and relpath() transforms it to an absolute path (i.e. prepends the working directory) before working out the result. From an actual windows box and C:\Users\me:

    >>> import ntpath
    >>> ntpath.relpath('C:\\', start='C:')
    '..\\..'
    >>> ntpath.relpath('C:\\', start='C:foo')
    '..\\..\\..'

    Here's pathlib for comparison's sake:

    >>> import pathlib
    >>> pathlib.PureWindowsPath('C:/').relative_to('C:')
    PureWindowsPath('/')
    >>> pathlib.PureWindowsPath('C:/').relative_to('C:foo')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/Users/barney.gale/code/cpython/Lib/pathlib.py", line 673, in relative_to
        raise ValueError("{!r} is not in the subpath of {!r}"
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ValueError: 'C:\\' is not in the subpath of 'C:foo' OR one path is relative and the other is absolute.

    miss-islington pushed a commit that referenced this issue Oct 28, 2022
    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
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir topic-pathlib type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants