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

Fix error message for ntpath.commonpath #117381

Closed
Tracked by #117361
nineteendo opened this issue Mar 29, 2024 · 4 comments
Closed
Tracked by #117361

Fix error message for ntpath.commonpath #117381

nineteendo opened this issue Mar 29, 2024 · 4 comments
Labels
OS-windows type-bug An unexpected behavior, bug, or error

Comments

@nineteendo
Copy link
Contributor

nineteendo commented Mar 29, 2024

Bug report

Bug description:

>>> import ntpath
>>> ntpath.commonpath(["/foo", "foo"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<frozen ntpath>", line 808, in commonpath
ValueError: Can't mix absolute and relative paths

/foo is not an absolute path, but a rooted path, which can't be mixed with not-rooted paths.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Windows

Linked PRs

@nineteendo nineteendo added the type-bug An unexpected behavior, bug, or error label Mar 29, 2024
@nineteendo nineteendo mentioned this issue Mar 29, 2024
16 tasks
@aisk
Copy link
Contributor

aisk commented Mar 30, 2024

I did some googling and found that Microsoft has a document which describes "relative rooted path": https://learn.microsoft.com/en-us/dotnet/api/system.io.path.ispathrooted?view=net-8.0#remarks

A relative rooted path specifies a drive, but its fully qualified path is resolved against the current directory.

So I think the relative rooted path should be something like C:foo (please notice that it's not C:/foo), but /foo still is an absolute path. And ntpath.isabs('/foo') returns True, so I think it's an absolute path.

@nineteendo
Copy link
Contributor Author

nineteendo commented Mar 30, 2024

ntpath.isabs('/foo') returns True

That was recently changed:

The error message could also be "Can't mix rooted and not-rooted paths" as /foo is rooted and foo is not.

@zooba
Copy link
Member

zooba commented Apr 2, 2024

How does this change make life better or easier for those encountering this message?

At first glance, it appears to be getting into overly precise terminology that doesn't help educate or correct the caller.

@nineteendo
Copy link
Contributor Author

The error message states: "Can't mix absolute and relative paths".
But /foo & foo are both relative according to ntpath.isabs():

Path Rooted? Absolute?
c:/foo Yes Yes
c:foo Yes No
/foo Yes No
foo No No

So, this error message is factually incorrect.
But /foo is rooted and foo is not, so that's what the new error message is in that case.
On a side note, it could be useful to add os.path.isrooted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-windows type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants