Final and prerelease control for package selection#13647
Final and prerelease control for package selection#13647notatallshaw merged 7 commits intopypa:mainfrom
Conversation
5b4b0c1 to
dbd03ab
Compare
|
Very nice - and very neeeded :) |
ichard26
left a comment
There was a problem hiding this comment.
First pass review. I'll need to take another look at this. Overall, logic looks good, but I'd like to take another look at the code.
| self.cmd_opts.add_option(cmdoptions.all_releases()) | ||
| self.cmd_opts.add_option(cmdoptions.only_final()) |
There was a problem hiding this comment.
As I noted in the --uploaded-prior-to PR, I believe it would be better if we grouped our command options more aggressively. Any command that accepts --no-binary (and friends) should also support --only-final and --all-releases for free.
This can be addressed as a follow up since the scope would be larger than any of these two PRs.
There was a problem hiding this comment.
I'm up for grouping, but given there's no convenient existing group I'd rather do it in a much smaller follow up PR.
ca2ffab to
44a24fe
Compare
|
This PR is ready for review/approval. FYI, for some reason my remote origin and the remote upstream branches got out of sync in a way that was difficult to fix, I think I've managed to resolve this now though and apply all the review suggestions, but I might do a rebase to make the commits a bit cleaner. The majority of the code in this PR is tests, the majority of the logic in this PR is CLI handling, the functional logic is actually quite simple. |
fea2cfc to
f6e934a
Compare
ichard26
left a comment
There was a problem hiding this comment.
Looks great! I'd like someone else to quickly give this a look over to make sure I didn't miss anything, but the tests are comprehensive enough to give me confidence this is ready to go.
I've pushed a commit to clean up the functional tests to use the modern test helpers I've recently added. I'm not sure about you, but I find the --no-cache-dir and --no-index flags distracting when trying to figure out what a test is doing.
If you're curious: review checklist
Feature quality
- Does the change pass the "least surprising behaviour" test?
- Has any required documentation been added?
- What are the potential edge cases and failure modes? Are they handled appropriately? In particular, is the error handling user-friendly?
Code quality
- Are tests comprehensive, readable, and well scoped?
- Are tests using best practices and modern patterns? (RS: I will address this myself)
- If the change adds tech debt, is there a plan for reducing/managing it?
Feel free to do so. I'm done reviewing so force pushes isn't going to be a problem 👍 |
0823e58 to
7000e51
Compare
7000e51 to
e18e237
Compare
FYI I will do one final pass myself, but I plan to merge this before 26.0 if there is no rejection. I see this PR as going hand in hand with #13746 which completes most of pip's conformance to the version specifier spec, as a consequence pre-releases will be selected in certain edge cases where they are not now, and the specification says tools SHOULD provide a way to exclude all pre-releases. |
|
FYI, packaging 26.0 is in, I will do a few smoke tests to ensure behavior works as expected with the new packaging, and then I plan to merge this this weekend. As a reminder |
Fixes #13221
Fixes #12470
Fixes #5503
This PR adds two new flags
--all-releasesand--only-final, they control package selection with the exact same semantics as--no-binaryand--only-binary, but for pre vs final releases instead of binary vs. source distributions.They are mutually exclusive with
--pre, as there are edge cases to do with the semantics of--preand these flags I didn't want to have to define. However, when only--preis used it is converted to the equivalent--all-releases :all:in the code, to keep the logic simple.The main feature I think users will be interested is only allowing final releases, to prevent pre-releases from being selected in any situation, e.g.