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 — bpo-36305: Fixes to path handling and parsing in pathlib by kmaork · Pull Request #12361 · python/cpython Conversation
|
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The is not the solution. "C:" is a drive-relative path. Of course it has a drive. We need to fix absolute to properly support drive-relative paths. See the FIXME comment in the code. It's not necessarily as simple as calling ntpath._getfullpatname, as suggested in the comment, unless we give up on preserving ".." components in Windows. (That wouldn't be a problem really since preserving ".." components serves no point in Windows, unlike POSIX.)
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.
You are right. I will modify the PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
This is not a bug. The resolved value of "c:" depends on its working directory, so joining it with "a" has to remain as "c:a". If we call |
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.
Note that "c:" is not a file stream path, and neither is "c:a". The system interprets "[A-Za-z]:" as a drive. If we want to create a file stream named "a" in a file named "c" in the working directory, we have to use a qualified path such as "./c:a".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
Which by the way is a separate bug in pathlib, if you want to look into it. For example, it's wrong for it to remove the explicit reference to the working directory in the following:
>>> p = Path('./c:a')
>>> str(p)
'c:a'
We gave it the path of a file stream, but it mistakenly handles it as a drive-relative path when getting the path as a string.
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.
... and yet,
>>> p = Path('c:a') >>> p WindowsPath('c:a') >>> Path('.') / p WindowsPath('c:a') >>> Path('./c:a') WindowsPath('c:a')
I don't think it's reasonable for Path('.') / Path('c:a') to give a different result than Path('./c:a'). I'm pretty sure I've seen it documented somewhere (although not in the official docs) that using Path('a/b/c') is a portable and shorter way of writing Path('a')/Path('b')/Path('c'), and the proposed behaviour violates that.
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.
It does make sense, and does seem to have been the assumption in pathlib (seeing the implementation for parse_parts).
But this is a wrong assumption, as sometimes a path part can be interpreted in more than one way (for example, c:a that can either be a drive-relative path or a file with a stream). That means that separating a path to its parts may cause a loss of information about the original path and lead to incorrect results.
Assuming that this assumption can't be true, I think that the current behavior -
WindowsPath('a') / WindowsPath('./c:b') returning WindowsPath('c:b') is surely worse than
WindowsPath('a') / WindowsPath('./c:b') returning WindowsPath('a/c:b') (which would be the case if my fixes are applied)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Part of this was fixed with bpo-34775, causing the merge conflict. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Hmm, I'm out of my depth here. @pfmoore @zooba Would you like to opine on the behaviour change in this PR? It's the first time I hear about NTFS data streams, myself... @kmaork It's taken some time, but would you like to rebase / merge this PR with git master to have a fresh CI run? |
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 think this PR has gotten stuck because the wanted behavior was not agreed upon. If we can settle it, someone (maybe me) will be able to implement it. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Pathlib's internal representation of paths is more complex than a single string. This allows operations like |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
Once #101667 lands, you should only need to change _format_parsed_parts() to fix the original bug.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| check(['a', 'Z:b', 'c'], ('Z:', '', ['Z:', 'a', 'b', 'c'])) | ||
| check(['Y:a', 'Z:b', 'c'], ('Z:', '', ['Z:', 'a', 'b', 'c'])) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right - if we switch drives mid-way through we should discard everything before. This is what os.path.join() does.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| equivalences.update({ | ||
| 'c:a': [ ('c:', 'a'), ('c:', 'a/'), ('/', 'c:', 'a') ], | ||
| './a:b': [ ('./a:b',) ], | ||
| 'a:b:c': [ ('./b:c', 'a:'), ('b:', 'a:b:c') ], |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above, I don't see why ('./b:c', 'a:') shouldn't resolve to a:.
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.