-
Notifications
You must be signed in to change notification settings - Fork 76
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
Also try mangling sdist urls to match pep625 #2794
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2794 +/- ##
==========================================
+ Coverage 75.00% 75.05% +0.05%
==========================================
Files 109 109
Lines 11156 11181 +25
==========================================
+ Hits 8367 8392 +25
Misses 2789 2789 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests? I know the tests are thing already. :/
how do we feel about moving a bunch of the |
What do you mean by that? W/e it is, a new PR please. |
I honestly don't get how this PR passes or works. Here are the existing tests for url transforms w/ pypi (which is way more complete than I recall!):
We already test at least part of the |
Oh you are munging hard-coded names. duh. Needs tests then for sure. |
Made #2797. Might do a little work there before coming back to here... and indeed, this (draft) PR needs some better stress tests, especially of bad/special/magic cases that categorically did change over time (PEP420 with |
We have pre-commit so it should be easy to solve linter errors. |
pre-commit.ci autofix |
I am struggling to find any in-the-wild examples that normalize |
That's fine. As long as something made up is in the tests, it will be fine I think. |
Thank you! |
This adds yet another URL to try, namely the basically-now-ubiquitous PEP625 form: