Repository navigation
Refactor the --color option implementation and add unit tests - #2458
Conversation
sirosen
added
the
bot:chronographer:skip
5dc4e0d to
d1ce0e1
Compare
d1ce0e1 to
1a86e82
Compare
1a86e82 to
80db760
Compare
1. Set `expose_value=False` so that `pip-compile` is not responsible for
handling the logic which more naturally belongs inside of the flag
implementation itself.
2. Implement that logic using a callback.
3. Add unit tests for the flag, in isolation from `pip-compile`.
f9f4538 to
cb6d2fb
Compare
| pytest.param(["--color"], True, id="enable"), | ||
| pytest.param(["--no-color"], False, id="disable"), | ||
| # last one wins semantics | ||
| pytest.param(["--color", "--no-color"], False, id="disable_overrides_enable"), |
There was a problem hiding this comment.
should we use kebab-case for IDs?
There was a problem hiding this comment.
🤷 Maybe?
I haven't thought very hard about it. I was using snake case so that it doesn't look like multiple parameters.
Do you want me to change it? I have no preference here, but want to be clear before I make an update. 😄
There was a problem hiding this comment.
Yeah, I figured you didn't want said confusion but it surprised me because I always use kebab-case for any non-python identifiers for distinction.
I can see that it can cause weirdness with multiple layers of parametrize, though.
I prefer dashes but won't insist. Your call.
|
Noticed a few integrations not reporting stuff. Attempted to fix some: #2498. Reopening the PR will hopefully report a few more checks, probably not all. |
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
The only reason I didn't approve earlier was that the missing coverage checks distracted me and I went on the side quest to fix them.. Feel free to make changes or merge as is.
Documentation build overview
37 files changed ·
|
|
Flaky failure in the merge queue: https://github.com/pypa/pip-tools/actions/runs/38812543452/job/116689071786#step:8:54. Scheduled again. |
2 participants
Set
expose_value=Falseso thatpip-compileis not responsible for handling the logic which more naturally belongs inside of the flag implementation itself.Implement that logic using a callback.
Add unit tests for the flag, in isolation from
pip-compile.No changelog needed. Yet another step towards complete test coverage. (See #2347 and #2264 for context.)
This is open as a draft because it modifies the same test file as #2457, and is therefore blocked on that PR.It's also a good checkpoint for this body of work and will be the last one I open for today. I did as many as I could, implementing a series of isolated changes, and this was the first naturally occurring collision between two of these improvements (they both need a unit test module for the CLI options).
Update: this should now be ready. 😄
Contributor checklist
changelog.d/(seechangelog.d/README.mdfor instructions) or the PR text says "no changelog needed".
Maintainer checklist
bot:chronographer:skiplabel.(following Semantic Versioning).