Skip to content
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

Implement --upload-before #12717

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

uranusjr
Copy link
Member

Close #6257. Still missing tests and docs, submitted for responses.

@pfmoore
Copy link
Member

pfmoore commented May 20, 2024

Better named --uploaded-before?

@uranusjr
Copy link
Member Author

Should probably be --exclude-uploaded-before. That’s sooo long though…

@pfmoore
Copy link
Member

pfmoore commented May 20, 2024

Hang on, does this only look at files uploaded before or after the provided date? My understanding of #6257 was that if you provided 1st January as the date, pip would ignore any files uploaded after that date.

So I'd write that as pip install foo --uploaded-before 01/01/2024.

Having said that, I agree it's still not completely obvious what's going on (that could be read as "install the latest version of foo that was uploaded before 1st Jan" and not constrain the dependencies...). UI is hard 🙁

@pradyunsg
Copy link
Member

pradyunsg commented May 20, 2024

Any reason to not use --exclude-newer instead here? It'll match what uv does, and we could also take the unambigous 2024-12-31 format.

@pfmoore
Copy link
Member

pfmoore commented May 20, 2024

Beyond "we didn't think of it"? Probably not...

Ah, it's what uv uses, that makes sense.

@uranusjr
Copy link
Member Author

I do kind of dislike that name though, it sounds too much like a boolean flag. --exclude-newer-than would be better. Not sure if it’s worthwhile to break compatibility.

@pfmoore
Copy link
Member

pfmoore commented May 20, 2024

I think the question of whether we should try to maintain option equivalence with uv is a bigger one, but one we should consider. I remain uncomfortable with the way that uv pip is sort-of-but-not-quite the same as pip, and I wish they'd not done that. But they did, and we have to live with that fact.

Thinking about --exclude-newer (which I hadn't before, beyond spotting that it was what uv used) I agree that it's not a good choice. It does indeed look like a boolean flag, rather than an option with an argument. If we want to be compatible with uv, I'd be inclined to work out what we think is the best name, and then ask them if they would be willing to change. If they say no, we can then decide whether we care more about compatibility or the option name.

Comment on lines +756 to +759
upload_before = datetime.datetime.fromisoformat(value)
# Assume local timezone if no offset is given in the ISO string.
if upload_before.tzinfo is None:
upload_before = upload_before.astimezone()
Copy link
Member

@pradyunsg pradyunsg May 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use the utc timezone here, unconditionally. That's what we'll be comparing against coming out of PyPI's responses AFAICT.

@ofek
Copy link
Sponsor Contributor

ofek commented Jun 11, 2024

Nice to see this happening!

My preference for the name would depend on its anticipated use. Say someone notes the date of successful environment creation or is using VCS metadata and wants to copy/paste. Would they be more or less likely to desire packages that were uploaded on that day?

My instinct would tell me that the inclusion of that date would be more desirable in which case I would prefer --exclude-newer-than. However, most people work in the morning and therefore it's possible a breaking change gets released in the evening or in a time zone further west. In that situation, the user would most certainly want to exclude that date in which case I would prefer --uploaded-before.

The former seems more intuitive but I just wanted to write down my thoughts 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Install packages up to a certain date
4 participants