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-30177: Fix misleading description of path.resolve() #1649

Closed
wants to merge 1 commit into from
Closed

bpo-30177: Fix misleading description of path.resolve() #1649

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 18, 2017

According to the documentation https://docs.python.org/3/library/pathlib.html#pathlib.Path.resolve
If strict is False, the path is resolved as far as possible and any remainder is appended without checking whether it exists.

The current behavior is not consistent with this, and only appends the first remainder.

This commit fixes the documentation

@mention-bot
Copy link

@Traceur759, thanks for your PR! By analyzing the history of the files in this pull request, we identified @eliben, @serhiy-storchaka and @marco-buttu to be potential reviewers.

@the-knights-who-say-ni
Copy link

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!

@seirl
Copy link
Contributor

seirl commented May 18, 2017

I was able to reproduce this bug, but I really don't think path.resolve() should only resolve the first segment.

This behavior is really confusing for multiple reasons, and I think the best way would be for it to work as currently documented, which is equivalent to the behavior of readlink -m.

If you want to check existence, you already have the strict attribute. The half-assed way of only expanding the existing segments doesn't have any use case as far as I can tell. It will silentely do the wrong thing without the developer being able to know it.

@marco-buttu
Copy link
Contributor

Thanks @Traceur759, I do not know whether there is a bug or the documentation is wrong. Maybe @zooba knows. In case the documentation is wrong, IMHO you can extend the patch with a short example, like this one:

.. doctest::

   >>> Path('/i/do/not/exist').resolve()
   PosixPath('/i')
   >>> Path('/i').resolve()
   PosixPath('/i')

@ghost
Copy link
Author

ghost commented May 18, 2017

It should not be a bug, inside of pathlib tests (path/to/cpython/Lib/test/test_pathlib.py), there is this piece of code starting at line 1491:

    self.assertEqual(str(p.resolve(strict=False)),
                     os.path.join(BASE, 'foo'))
    p = P(BASE, 'foo', 'in', 'spam')

As you can see, the test expects the rest to be omitted, no way of being a bug inside the test.

This behavior raises lots of controversy and most likely should be discussed with the upstream, but for now it seems like the docs should be edited.
You can see quite a big conversation on this topic here:
https://bugs.python.org/issue19717

@marco-buttu
Copy link
Contributor

Thanks @Traceur759. About adding the example I proposed in the previous message, let's wait for a core-dev opinion.

@seirl
Copy link
Contributor

seirl commented May 18, 2017

I'd like to add that in Windows + Python 3.6, the behavior matches the documented one (which is the correct one, imho).

>>> pathlib.Path('a/b/c/d').resolve(strict=False)
WindowsPath('a/b/c/d')

@seirl
Copy link
Contributor

seirl commented May 18, 2017

For what it's worth, if this can have any impact in the decision making, this bug was the cause of an important downtime in our organization, because someone needed to normalize a path that was later passed to a .mkdir(parents=True), and it was, in some cases, silently doing the mkdir of a wrong path instead of creating all the parents (aka the behavior that is currently documented).

@richardcooper
Copy link

I would argue that the documentation is correct in this case and there is a bug in the code.

The strict flag was added as a result of bpo-19717. The decision on what to do when strict=False seems to come in https://mail.python.org/pipermail/python-ideas/2016-September/042203.html where Guido says:

I would prefer it if Path.resolve() resolved symlinks until it hits
something that doesn't exist and then just keep the rest of the path
unchanged.

The documented behaviour also seems much more useful than the current behaviour. I encountered this bug in much the same was as @seirl - I needed a path which was both absolute and useable for .mkdir()

@zooba
Copy link
Member

zooba commented May 31, 2017

Let's keep the discussion on http://bugs.python.org/issue30177 please, and use this thread solely for code review.

@pitrou
Copy link
Member

pitrou commented May 31, 2017

I'd like this PR to be rejected, as this clearly is not the desirable solution to the original issue.

@serhiy-storchaka
Copy link
Member

Seconded.

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

Successfully merging this pull request may close these issues.

None yet

9 participants