refactor: have a single function for normalized PyPI package names #1329
Conversation
| "friendly.bard", | ||
| "friendly_bard", | ||
| "friendly--bard", | ||
| "FrIeNdLy-._.-bArD", |
There was a problem hiding this comment.
Can we add an empty string here please? And maybe something that is not a string?
There was a problem hiding this comment.
I am not sure I can do that easily because I am not sure rules_testing supports failure testing yet and I am not sure if passing a failure function to the normalize_name would be a good idea. Because this is a private function I am not sure how much benefit there is in doing such testing.
There was a problem hiding this comment.
Failure testing a unit test is possible, but annoying to do. You basically have to switch over to treating it like an analysis test (the same as skylib's unittest): implement a rule to call your code that fails, instantiate the target as the target under test, then check for that failure in the target under test.
(Fabian and I tried kicking around ideas for how we could somehow make use whats available today to make it easier, but couldn't come up with anything).
I'm fine with the tests as-is, fwiw.
python/private/normalize_name.bzl
Outdated
| Normalize a PyPI package name to allow consistent label names | ||
| """ | ||
|
|
||
| # Keep in sync with python/pip_install/tools/bazel.py |
There was a problem hiding this comment.
Can we rewrite bazel.py, so that we only have this code here? I do not want that rewrite in this PR, but if we can I would love an issue opened.
There was a problem hiding this comment.
It is possible but not entirely easy, so created #1330. Let's discuss the technical details there.
| "friendly.bard", | ||
| "friendly_bard", | ||
| "friendly--bard", | ||
| "FrIeNdLy-._.-bArD", |
There was a problem hiding this comment.
Failure testing a unit test is possible, but annoying to do. You basically have to switch over to treating it like an analysis test (the same as skylib's unittest): implement a rule to call your code that fails, instantiate the target as the target under test, then check for that failure in the target under test.
(Fabian and I tried kicking around ideas for how we could somehow make use whats available today to make it easier, but couldn't come up with anything).
I'm fine with the tests as-is, fwiw.
|
@aignas I approved the PR but CI is failing |
|
@aignas, I also included a doc change that Richard recommended, but that is not why CI is failing. |
aignas
force-pushed
the
refactor/normalize-name
branch
from
81e106b
to
86dc036
Compare
|
Thanks guys for the review. The |
Before this PR there were at least 2 places where such a helper function
existed and it made it very easy to make another copy. This PR introduces a
hardened version, that follows conventions from upstream PyPI and tests have
been added.
Split from #1294, work towards #1262.