Support relative values/durations in --uploaded-prior-to#13837
Support relative values/durations in --uploaded-prior-to#13837notatallshaw merged 10 commits intopypa:mainfrom
--uploaded-prior-to#13837Conversation
|
Before I review the code can you expand on the design choices you made here given the discussion in #13674, let us know if you purposely made a choice and why, or if it was arbitrary:
|
|
Thanks. I answered the questions below. I can see how this could be simplified, and look forward to feedback. Why did you create a new option rather than using the existing --uploaded-prior-to option?The parameter name was chosen from the (p)npm tool for JS: --min-release-age 2, where plain integer translates to days. I would like to have it consistent across tools, to make it simple for users. --uploaded-prior-to maps more to the --before, and I related both to an absolute time stamp. Why did you create new unitsThe units are inspired by the "friendly" duration from "uv". The pnpm tool also has a minimumReleaseAge. While npm uses days as unit, pnpm uses to minutes. To make the input more human readable, and to support different time precision, I started with the user friendly units. I am not sure whether many users would like the ISO duration that is discussed in #13674. npm has several pull requests that have a simple unit. |
|
I recommend reading to the end of #13674, there is a lot of design discussion there and seems like we were heading towards consensus about what to support. I don't think we should consider JS or other ecosystems above the Python ecosystem, so going forward with the option pip has selected and a subset of the format(s) that uv has chosen seems like it'd be the most consistent experience for Python users. |
|
In the issue, to me there is no consensus on the details yet. I asked in that issue as well. The parameter supports plain integers as days, or units similar to uv's "friendly" format. I decided not to overload "--uploaded-prior-to", as that parameter implies an absolute time stamp and I find "--uploaded-prior-to=3days" misleading. It is not clear to me whether you already agreed on overloading "--uploaded-prior-to" or agree to have a separate parameter. I can change the parameter name to "exclude-newer", if consistency to "uv" is more important than to npm. For the implementation, could we go with an incremental approach and add the parameter with an integer unit for e.g. days first, and later add support for strings with further units based on user feedback? |
e1aa2ee to
bd9b1bc
Compare
|
I changed the logic to extend the existing parameter Testing DoneUsing pip with the --uploaded-prior-to and the value given in the test case, to then show the installed version of the certifi package. |
notatallshaw
left a comment
There was a problem hiding this comment.
This needs a section in the user guide to carefully reflect how to communicate this following the discussion: #13674
It would come in the "Filtering By Upload Time" section https://pip.pypa.io/en/stable/user_guide/#filtering-by-upload-time
bd9b1bc to
7ae71ef
Compare
As a user, I want to be able to set a relative time to specify the package versions to update. This commit extends the --uploaded-prior-to option to accept relative time spans in ISO format for duration in days (e.g., 'P3D'). The callback parses the given string into a time duration, and next computes and absolute timestamp. Then, we use the logic already present in uploaded_prior_to to process the absolute time. Unit tests are extended accordingly. Signed-off-by: Norbert Manthey <nmanthey@amazon.de>
Add towncrier news fragment for issue pypa#13674.
7ae71ef to
cda89ed
Compare
|
I dropped the support for lower case letters, and adapted the unit tests accordingly. |
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@stefanv I don't disagree with anything you say, but please keep high level discussion of this feature in the issue, I want to focus PR discussions on the details of the implementation. To implement something more nuanced than uv or npm would need agreement in the issue |
sethmlarson
left a comment
There was a problem hiding this comment.
🚀 Can't wait to use this feature!
|
Yeah, this is looking good now, I'm going to review the wording a bit more, I might push my own commit. |
|
I've updated the language a little, including a warning about supply chain attacks vs. security vulnerabilities, made the matching regex ASCII only, and added a test that P0D on CLI works as expected when a larger value is set as an env. |
sethmlarson
left a comment
There was a problem hiding this comment.
I believe we should be using P7D instead of P1D as our example value. Examples have a funny way of becoming what people actually use in the wild :)
If this feature is being used for security we need to account for the human processes happening behind the scenes which are subject to weekends and vacations.
--uploaded-prior-to
|
I further updated the language to match @zanieb feedback. I also discussed with @sethmlarson on discord and I pushed back a bit on a 7 day suggestion, we've compromised on 3 days, to account for weeekends, hopefully that works well as a suggestion. |
|
Unless there are any objections, or requests to pause for further review, I'm going to merge this on Friday. |
5950d58 to
a0a64b1
Compare
a0a64b1 to
7544d63
Compare
|
I made one more minor change to the user guide, I added a |
|
@nmanthey Thanks for this PR and persistence keeping this PR up to date with the latest thinking on the UX for this feature. |
|
Thanks for picking it up and making it available! I'll see whether the package exclusion filter, similarly to what uv provides, would be a useful addition to user workflows. Let me know if you'd be interested in this. |
|
Please open a new issue and outline the use case, pip maintainers will need to agree the value of the use case outweighs the cost of adding yet another option. |
Fixes #13674
As a user, I want to be able to set a relative time to specify the package versions to update. This feature is present in package managers of other repositories, e.g. in npm, as --min-release-age.
This commit introduces a new --min-release-age option to accept relative time spans (e.g., '7d', '168h', '10080m') as an alternative to --uploaded-prior-to.
The callback parses the given time stamp into an absolute time and next uses the logic already present via the uploaded_prior_to. Parsing supports d/days, h/hours, m/minutes, w/weeks as units. Unit tests are added accordingly, following the test pattern of --uploaded-prior-to.
Related issue #13674
Testing Done
Unit tests check the data parsing code similar to testing for the existing "--uploaded-prior-to" flag.
In addition, I tested the implementation in a local venv:
pip install certifipip install --min-release-age=10 certifipip install --min-release-age=365 certifiTesting Executed
Setup and install current pip
Check versions of certifi package
Install version from a year ago
Uninstall and install version from 10 days ago
Uninstall and install latest version
This change is