Skip to content

gh-74453: Deprecate os.path.commonprefix - #144436

Merged
hugovk merged 7 commits into
python:mainfrom
sethmlarson:deprecate-os-path-commonprefix
Feb 5, 2026
Merged

gh-74453: Deprecate os.path.commonprefix#144436
hugovk merged 7 commits into
python:mainfrom
sethmlarson:deprecate-os-path-commonprefix

Conversation

@sethmlarson

@sethmlarsonsethmlarson commented Feb 3, 2026

Copy link
Copy Markdown
Contributor

Follow-up from #144401, this PR moves to deprecate os.path.commonprefix() in favor of the correctly behaving os.path.commonpath() and the behavior-describing string.commonprefix().

I'm not sure if this is all I have to do to deprecate an API, if there is more documentation that needs to happen let me know.


📚 Documentation preview 📚: https://cpython-previews--144436.org.readthedocs.build/

@picnixzpicnixz left a comment

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.

You need a What's New entry for that.

Comment threadDoc/library/os.path.rst Outdated
Comment threadDoc/library/string.rst Outdated
Comment threadLib/genericpath.py Outdated
Comment threadLib/string/__init__.py Outdated
Comment threadLib/string/__init__.py Outdated
Comment on lines +61 to +62
import os
m = tuple(map(os.fspath, m))

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.

I'm not really fond of this. string.commonprefix() should be specific to strings and should not care about pathlike objects.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, this is only done for backwards compatibility. I would be happy to get rid of it, but I figured that this would be not allowed considering we're doing a "rename", not a new API.

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.

There is no backwards compatibility here. We create a new function in a module that is not related to filepaths at all. I do not consider this as a rename, I really consider this as a new API. Instead, I would keep the implementation of os.path.commonpath as is and add in a follow-up PR string.commonprefix which does not do the fspath computation. We then document that os.path.commonpath should be reimplemented as string.commonprefix(tuple(map(os.fspath, m))) if needed.

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.

I believe this new string.commonprefix() should be a direct, one-to-one, drop-in replacement for the old os.path.commonprefix().

We're not doing anyone favours by changing the behaviour and making them [have to decide whether they need to] jump through extra hoops.

Chances are they should be using os.path.commonpath() instead, but if not, let them just replace the module name and keep the same old behaviour.

I doubt there's significant performance improvement from changing it either.

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.

For me this isn't about performance, it's about semantics. string contains string-related utilities, not path-related ones. I am really opposed to having string.commonprefix being smart here. I know that we are not necessarily doing the user a favor but I don't want to have unrelated utilities in string. It is, for me, the wrong module for that.

@hugovkhugovk left a comment

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.

Please also list in Doc/deprecations/pending-removal-in-future.rst.

https://docs.python.org/dev/deprecations/index.html#pending-removal-in-future-versions

Comment threadDoc/library/string.rst Outdated
Comment threadLib/string/__init__.py Outdated
Comment on lines +61 to +62
import os
m = tuple(map(os.fspath, m))

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.

I believe this new string.commonprefix() should be a direct, one-to-one, drop-in replacement for the old os.path.commonprefix().

We're not doing anyone favours by changing the behaviour and making them [have to decide whether they need to] jump through extra hoops.

Chances are they should be using os.path.commonpath() instead, but if not, let them just replace the module name and keep the same old behaviour.

I doubt there's significant performance improvement from changing it either.

sethmlarsonand others added 2 commits February 3, 2026 12:28
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@sethmlarson

Copy link
Copy Markdown
ContributorAuthor

Alright, most recent two commits should address all review comments. I have preserved backwards compatibility in the drop-in replacement string.commonprefix().

@sethmlarson
sethmlarson requested review from hugovk and picnixz and removed request for picnixzFebruary 3, 2026 18:33
Comment threadDoc/deprecations/pending-removal-in-future.rst Outdated
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

@picnixzpicnixz left a comment

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.

I am sorry, but I do not want string.commonprefix to be equivalent to os.path.commonprefix. Semantically it doesn't make sense to have an os-related utility in string.

In addition, we only talk about "strings" in the documentation. Having an implicit os.fspath conversion should be documented otherwise. If we want to mimic os.path.commonpath in the first place, I do not see why we can't just keep it but soft-deprecate it.

Note that commonprefix does not care about the fact that we are strings. It also works for any sequence, as long as the elements can be ordered (e.g., we can use commonprefix for list of lists):

>>>os.path.commonpath([[1], [1,3]])
[1]

Since we expose it in string, anyone using a non-string as the first argument will not be able to make it work (because we raise a TypeError in os.fspath). So I would expect a string-like object to be acceptable but because of os.fspath it wouldn't be. This would be contrary to what the string module stands for.

If we want a commonprefix utility, I would suggest adding one to itertools, and use the latter. I believe it makes more sense to have something related to commonprefix out there. Or for strings only, have it in difflib.

Comment threadDoc/library/string.rst Outdated
@bedevere-app

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

@sethmlarson

Copy link
Copy Markdown
ContributorAuthor

Because I want to prioritize warning users about this function, I am okay with not providing a string.commonprefix() function and simply raising a DeprecationWarning in this PR. I want to avoid this warning getting delayed further, as has happened for the past ~30 years since the confusing behavior has been reported.

sethmlarsonand others added 2 commits February 3, 2026 21:56
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@sethmlarson
sethmlarsonforce-pushed the deprecate-os-path-commonprefix branch from d4c0bb4 to 166c39dCompareFebruary 3, 2026 23:06
@sethmlarson

sethmlarson commented Feb 3, 2026

Copy link
Copy Markdown
ContributorAuthor

@picnixz@hugovk I have removed string.commonprefix in 166c39d, I will happily revert or change this if there is a suggestion that should be completed in this PR.

(I have made the requested changes; please review again)

@sethmlarson

Copy link
Copy Markdown
ContributorAuthor

I have made the requested changes; please review again

@bedevere-app

Copy link
Copy Markdown

Thanks for making the requested changes!

@hugovk, @picnixz: please review the changes made to this pull request.

@bedevere-app
bedevere-appBot requested a review from hugovkFebruary 4, 2026 15:46

@vstinnervstinner left a comment

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.

LGTM.

Comment threadLib/posixpath.py
path_list = path_tail.split(sep) if path_tail else []
# Work out how much of the filepath is shared by start and path.
i = len(commonprefix([start_list, path_list]))
i = len(genericpath._commonprefix([start_list, path_list]))

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, this call is safe since it pass two lists of strings, and not two strings.

@hugovkhugovk left a comment

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.

Thanks!

@picnixz

Copy link
Copy Markdown
Member

The question still remains about whether to expose string.commonprefix as something working on strings only (not paths), itertools.commonprefix (as working on any sequences) or the new seqtools module (I believe the discussion is stalled).

This would be a natural addition to that module, but that would also be a natural addition to more_itertools or another PyPI package dedicated to sequences utilities. OTOH, it'd still be possible to have it in itertools though supporting arbitrary iterables would make the implementation more complex.

OTOH, difflibcould be used as an alternative (I mean, difflib seems to work for any sequence, except when we want to format the change but otherwise the diff algorithm looks generic?)

@vstinner

Copy link
Copy Markdown
Member

The question still remains about whether to expose string.commonprefix as something working on strings only (not paths), itertools.commonprefix (as working on any sequences) or the new seqtools module (I believe the discussion is stalled).

I'm not sure that we have to provide a replacement function. os.path.commonpath() should cover most usages.

@hugovk

Copy link
Copy Markdown
Member

Please open a new issue or discussion if you'd like a new method. We decided not to provide a replacement here, so it can be considered independently.

@picnixz

Copy link
Copy Markdown
Member

I personally don't think it's necessary to provide a pure replacement but I don't know whether Seth wants to

@sethmlarson

sethmlarson commented Feb 5, 2026

Copy link
Copy Markdown
ContributorAuthor

I don't feel that we need an exact replacement. string.commonprefix() only operating on strings seems fine, but if it's not an exact replacement then it can happen outside this PR since it's unrelated then.

@hugovk
hugovk merged commit 957f9fe into python:mainFeb 5, 2026
52 of 55 checks passed
@sethmlarson
sethmlarson deleted the deprecate-os-path-commonprefix branch February 5, 2026 20:37
thunder-coding pushed a commit to thunder-coding/cpython that referenced this pull request Feb 15, 2026
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
anki-code pushed a commit to xonsh/xonsh that referenced this pull request Feb 19, 2026
…on3.15 (#6102)
python/cpython#144436 deprecates `os.path.commonprefix` in 3.15, causing
a `DeprecationWarning` each time when using completion
i copied the implementation of `commonprefix`
(https://github.com/python/cpython/blob/957f9fe162398fceeaa9ddba8b40046b8a03176d/Lib/genericpath.py#L115-L129)
and removed the unused `fspath`, as xonsh only uses `commonprefix` with
`str | RichCompletion`, not `PathLike`
i first thought about putting the function into the `xonsh.tools`
module, but one of the places where `commonprefix` is used in
`xonsh/completers/bash_completion.py`, which is intended to be usable as
a standalone script, and adding an `import xonsh.tools` increased the
startup time by a significant amount, so i decided to create a new
helper module
## For community
⬇️ **Please click the 👍 reaction instead of leaving a `+1` or 👍
comment**
github-merge-queueBot pushed a commit to meltano/meltano that referenced this pull request Mar 20, 2026
…gments (#9910)
## Description
<!-- Describe the changes introduced by this PR -->
SSIA.
## Checklist
- [x] I have read the [contribution
guide](https://docs.meltano.com/contribute/merge/)
### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this
PR, please change below checkbox to `[X]` followed by the name of the
tool, uncomment the "Generated-by". See the agentic coding section of
the contribution guide for details:
https://github.com/meltano/meltano/blob/main/CONTRIBUTING.md#agentic-coding
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the agentic coding
guidelines](https://github.com/meltano/meltano/blob/main/CONTRIBUTING.md#agentic-coding)
-->
## Related Issues
- pygments/pygments#3039
- python/cpython#74453
- python/cpython#144436
## Summary by Sourcery
Adjust test configuration to handle a new Python 3.15 deprecation
warning from pygments and scope a Pyo3 compatibility flag to nox-managed
environments.
Build:
- Add a pytest warning filter in pyproject.toml to ignore the pygments
DeprecationWarning about os.path.commonprefix on Python 3.15.
CI:
- Remove the global PYO3_USE_ABI3_FORWARD_COMPATIBILITY environment
variable from the GitHub Actions test workflow and rely on environment
configuration within nox sessions instead.
Tests:
- Set PYO3_USE_ABI3_FORWARD_COMPATIBILITY within the nox _uv_sync
session environment so that test environments retain the required Pyo3
ABI compatibility setting.
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
ljfp pushed a commit to ljfp/cpython that referenced this pull request Apr 25, 2026
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Sign up for freeto 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.

5 participants

@sethmlarson@picnixz@vstinner@hugovk@StanFromIreland