Skip to content

Replace deprecated os.path.commonprefix with os.path.commonpath#13847

Merged
ichard26 merged 1 commit intopypa:mainfrom
hugovk:main
Mar 22, 2026
Merged

Replace deprecated os.path.commonprefix with os.path.commonpath#13847
ichard26 merged 1 commit intopypa:mainfrom
hugovk:main

Conversation

@hugovk
Copy link
Copy Markdown
Contributor

@hugovk hugovk commented Mar 14, 2026

Copy link
Copy Markdown
Member

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, although I have one question about the change in auth.py.

Comment on lines +328 to +335
key=lambda candidate: len(
commonpath(
[
parsed_url.path,
candidate.path,
]
)
),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem exactly behaviour equivalent to the old logic? Although I'm not sure what the old logic is supposed to do either. Pick the least deep common prefixing path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mostly the same, and the case it's different doesn't matter.

Each of these lambdas is returning an integer to show how close each of the candidate URLs are to the input one, then using the integer to sort the list and pick the first one as the closest match.

  • Before, commonprefix matched by character. So given something like "/path/path2/" and "/path/path3/", it'd give "/path/path". Then rfind("/") gives us the index of that last "/", or 5.

  • After, commonpath matches by full path parts. With the same inputs, we get "/path". len of that is also 5.

There is one slight difference, but it doesn't matter, and it's for the case when there isn't really any match.

  • If we have two paths like "/path/path2/" and "/path4/path3/", before commonprefix would give "/path", and `rfind("/") would give the index of the only "/", or 0.

  • Afterwards, commonpath gives us "/", whose len is 1.

That's okay, because in both cases, the shortest value we get for a meaningful common match is 2. For example, with "/p/path2/" and "/p/path3/":

  • Before: commonprefix(["/p/path2/", "/p/path3/"]) -> "/p/path" -> rfind("/") -> 2

  • After: commonpath(["/p/path2/", "/p/path3/"]) -> "/p" -> len -> 2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. I did misread the original code, so that didn't help. Thanks!

@ichard26 ichard26 merged commit ed33af5 into pypa:main Mar 22, 2026
28 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants