Allow local date for inline exclude newer #6562
Conversation
| impl<'de> serde::Deserialize<'de> for ExcludeNewer { | ||
| fn deserialize<D>(deserializer: D) -> Result<ExcludeNewer, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let s = String::deserialize(deserializer)?; | ||
| FromStr::from_str(&s).map_err(serde::de::Error::custom) | ||
| } | ||
| } |
There was a problem hiding this comment.
cc @BurntSushi I remember some recent discussion about local dates?
zanieb
added
documentation
|
@hauntsaninja on further thought, isn't it wrong to allow a local date for I think this notably differs from |
|
It's a fair concern! The use case I'm thinking of is "I wrote a script a few years ago and I want it to mostly run the same, maximising convenience / bang for buck". My guess is this use case will be much more common than inter-timezone distribution. We could also resolve different dependencies for different people depending on arch or python version as well, it's why I phrased this as "improving" reproducibility. |
|
I feel like if we want to maximize convenience we should allow emitting an exclude newer pin (in UTC) via a command or, even better, we should be able to write a lock to scripts. Hm. If you're willing to separate the change from the docs, we can merge the docs right away. I think we'll wait for @BurntSushi to be back from vacation to make a decision on the local dates. |
There was a problem hiding this comment.
I think this is making the serde::Deserialize trait implementation on ExcludeNewer consistent with its FromStr trait implementation.
I am still pretty iffy on supporting local dates here though. When I added support for it to ExcludeNewer, I was thinking about its use on the command line or in user-specific configuration files. But this seems to be expanding its scope to a point where I think it actually becomes problematic. If, for example, exclude-newer = "2023-10-16" is put into a script and then shared with someone else, that date may be interpreted as an entirely different instant than is intended. It might be better to just keep it restricted to RFC 3339 timestamps. Although, the difference in the Deserialize and FromStr trait implementation seems like a footgun.
|
For completeness, one alternative here is to parse an RFC 9557 timestamp (an extension to RFC 3339). So you could write something like |
|
Related: #6820 |
|
Actually one possibly silly thought, should we make the short form just be a UTC timestamp? In the short form I think it's arguably already a little unclear what the boundary semantics are (e.g. This is also nice if you have a cloud machine on UTC time and you want to reproduce a command using the short form on your local machine. It's technically a breaking change, but it seems maybe saner, and I have a hard time thinking about the user profile who would be broken by it. |
|
Anyway, sounds like I should split the PRs regardless: #6831 for docs! |
hauntsaninja
changed the title
This is what it was originally, but I changed it. It's generally bad juju to just assume UTC for an unadorned date. I think we either don't support an unadorned date or we assume it's in the user's local time zone. But in this case, "user's local time zone" creates a configuration that isn't self-contained. In terms of the time for a date without a time, it's common practice to interpret it as the first instant on that date (which, interestingly, may not be midnight). It's probably best to just required an RFC 3339 timestamp here. |
|
Sounds good! |
Resolves #6555