gh-74453: Deprecate os.path.commonprefix #144436
Conversation
sethmlarson
requested review from
StanFromIreland,
hugovk,
picnixz and
vstinner
picnixz
left a comment
picnixz
left a comment
There was a problem hiding this comment.
You need a What's New entry for that.
| import os | ||
| m = tuple(map(os.fspath, m)) |
There was a problem hiding this comment.
I'm not really fond of this. string.commonprefix() should be specific to strings and should not care about pathlike objects.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
hugovk
left a comment
hugovk
left a comment
There was a problem hiding this comment.
Please also list in Doc/deprecations/pending-removal-in-future.rst.
https://docs.python.org/dev/deprecations/index.html#pending-removal-in-future-versions
| import os | ||
| m = tuple(map(os.fspath, m)) |
There was a problem hiding this comment.
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.
Co-authored-by: Bénédikt Tran <[email protected]> Co-authored-by: Hugo van Kemenade <[email protected]>
|
Alright, most recent two commits should address all review comments. I have preserved backwards compatibility in the drop-in replacement |
sethmlarson
requested review from
hugovk and
picnixz
and removed request for
picnixz
Co-authored-by: Hugo van Kemenade <[email protected]>
There was a problem hiding this comment.
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 are 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.
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
|
Because I want to prioritize warning users about this function, I am okay with not providing a |
Co-authored-by: Bénédikt Tran <[email protected]>
4 participants
Follow-up from #144401, this PR moves to deprecate
os.path.commonprefix()in favor of the correctly behavingos.path.commonpath()and the behavior-describingstring.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/