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

use packaging.utils.canonicalize_name() #6022

Merged

Conversation

dimbleby
Copy link
Contributor

@dimbleby dimbleby commented Jul 17, 2022

poetry currently has two different versions of canonicalize_name() floating around, which is slightly absurd - and what's worse, they don't even agree.

Since python-poetry/poetry-core#328, poetry-core has an implementation that agrees with the implementation in packaging - but poetry has its own implementation that continues to disagree with that. Sometimes poetry uses one of these, and sometimes the other.

Let's treat the version from packaging as the canonical canonicalize_name(), and get rid of both of the re-implementations.

I don't know that this fixes - or breaks! - anything in particular; the unit tests don't detect a difference. But two differing implementations is surely an accident waiting to happen.

Aside from being one-true-implementation, a potential advantage of the packaging version is that it returns a NormalizedName: which is a newtype around string, rather than a plain string. That opens the door to future work in which the typechecker can verify that places which expect canonicalized names do indeed get them.

(Will follow up in poetry-core, but this one will need merging first since poetry sometimes uses the poetry-core version).

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

This LGTM. However, I wonder if it makes sense to re-export the method in poetry-core? Given your observations about alignment of the two projects, I feel like it might be good to ensure the same code is always used in the case of a packaging version mismatch.

@dimbleby
Copy link
Contributor Author

I wonder if it makes sense to re-export the method in poetry-core?

This seems excessive to me. Any change to this method in packaging would be enormously breaking and not PEP-503 compliant.

This method hasn't been changed (bar type annotations) since introduction at pypa/packaging#60, and I don't expect that it ever will be.

@neersighted neersighted merged commit ff8ae32 into python-poetry:master Jul 18, 2022
@dimbleby dimbleby deleted the just-one-canonicalize-name branch July 18, 2022 17:24
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants