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

canonicalize package name correctly #328

Merged
merged 1 commit into from
May 10, 2022

Conversation

dimbleby
Copy link
Contributor

Resolves: python-poetry/poetry#5474, python-poetry/poetry#3132, python-poetry/poetry#3200

looks like this regex was probably just badly copied from https://github.com/pypa/packaging/blob/ba124324e866e518ebfe0378a25b6ba4816e5880/packaging/utils.py#L27

  • Added tests for changed code.
  • Updated documentation for changed code.

dimbleby added a commit to dimbleby/poetry that referenced this pull request Apr 21, 2022
dimbleby added a commit to dimbleby/poetry that referenced this pull request Apr 21, 2022
@dimbleby
Copy link
Contributor Author

downstream tests fixed in python-poetry/poetry#5475

dimbleby added a commit to dimbleby/poetry that referenced this pull request Apr 21, 2022
@dimbleby
Copy link
Contributor Author

I have just spotted that there is history here at #100 and #132.

#132 doesn't say what the "unintended side effects" were that caused this to be reverted. I doubt that they were worse than the infinite loops that this fixes.

That makes my guess that this was mis-copied from packaging.utils look less likely; but in that case it might be reassuring to see that they have the same solution as in this MR.

@radoering
Copy link
Member

#132 doesn't say what the "unintended side effects" were that caused this to be reverted. I doubt that they were worse than the infinite loops that this fixes.

At least it says that the canonicalized names were propagated in too many places and that the original names should be kept longer. Thus, I suppose we would have to evaluate each call of canonicalize_name, remove some of them and maybe add some new ones if we want to be thorough about this. 🙁

@abn Do you remember why the original PR was reverted? I assume it wasn't just for cosmetic reasons?

@abn
Copy link
Member

abn commented May 2, 2022

I suspect there were a few different issues that piled up which may have already been resolved. Based on talking with sdispater, seems the issue was dependency resolution was breaking in some cases. Like, for example A depends on A-X and A.X is provided on PyPI. The downstream test failures might be an indicator, but curosory look seems to imply the issue might be more test data than actual resolution failures. We should probably get the poetry tests fixed in a forwards compatible manner and add some test cases around this - hopefully that will reveal any real issues.

I am going to suggest that we move forward with this after the tests are fixed and fix any regressions as they come.

@sonarcloud
Copy link

sonarcloud bot commented May 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.4% 0.4% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants