Skip to content

Support dates for exclude-newer tool setting#8553

Closed
manzt wants to merge 1 commit intoastral-sh:mainfrom
manzt:manzt/more-flexible-exclude-newer
Closed

Support dates for exclude-newer tool setting#8553
manzt wants to merge 1 commit intoastral-sh:mainfrom
manzt:manzt/more-flexible-exclude-newer

Conversation

@manzt
Copy link
Contributor

@manzt manzt commented Oct 25, 2024

Summary

hey! I was poking around with the tool support for exclude newer in juv (think this would be awesome to use to make Jupyter notebooks more standalone/reproducible) and ran into an issue.

The settings documentation for exclude-newer says that it supports both dates and timestamp.

However, with uv v0.4.26:

uv init foo
cd foo
echo "[tool.uv]\nexclude-newer = '2022-01-01'" >> pyproject.toml
uv run hello.py
#  warning: Failed to parse `pyproject.toml` during settings discovery:
#   TOML parse error at line 9, column 17
#     |
#   9 | exclude-newer = "2022-01-01"
#     |                 ^^^^^^^^^^^^
#   failed to find time component in "2022-01-01", which is required for parsing a timestamp

I believe this is because the derive'd Deserialize from jiff only supports timestamps and doesn't use the more flexible FromStr implemention implemented for ExlcudeNewer

Test Plan

Added a test with a date string

@manzt manzt force-pushed the manzt/more-flexible-exclude-newer branch from 91f0be2 to f5b3502 Compare October 25, 2024 03:14
@manzt manzt changed the title Support dates for exclude-newer tool setting Support dates for exclude-newer tool setting Oct 25, 2024
@manzt manzt force-pushed the manzt/more-flexible-exclude-newer branch from f5b3502 to 2e795ff Compare October 25, 2024 03:28
@zanieb
Copy link
Member

zanieb commented Oct 25, 2024

What timezone does this use? UTC?

I'm not sure if this is quite the same, but we discussed something like this in #6562 and decided against the change.

@manzt
Copy link
Contributor Author

manzt commented Oct 25, 2024

The docs say:

Accepts both RFC 3339 timestamps (e.g., 2006-12-02T02:07:43Z) and local dates in the same format (e.g., 2006-12-02) in your system's configured time zone.

So I'm guessing whatever the system's time zone is.

@manzt
Copy link
Contributor Author

manzt commented Oct 25, 2024

Looking at the from_str implementation for ExcludeNewer, it looks like that way:

            .and_then(|date| date.to_zoned(TimeZone::system()))

https://github.com/manzt/uv/blob/2e795ffc4b4976584b5bb67b6860681e6492f687/crates/uv-resolver/src/exclude_newer.rs#L34-L68

If that's not the intention, than maybe we should change the example in the docs.

@zanieb
Copy link
Member

zanieb commented Oct 25, 2024

Yeah I think we might want to change the docs or, if we want to accept a simple date, use UTC. I think the intent is that we want a stable time in exclude-newer encoded in files. For example, if someone clones your repository they shouldn't get a different resolution because they have a different system time.

@BurntSushi has the most context on this though.

@manzt
Copy link
Contributor Author

manzt commented Oct 25, 2024

For example, if someone clones your repository they shouldn't get a different resolution because they have a different system time.

Makes total sense. I'm thinking the main "push" for flexibility here (as mentioned in #6562) is convenience (i.e., actually typing out the timestamp is annoying), but not that someone adamantly wants ambiguity. Maybe something to consider is just a convenient high-level API for adding this setting correctly:

uv add --timestamp
uv add --script foo.py --timestamp
uv add --timestamp=2024-01-01 

General idea is that you could have something conveniently resolve the users local time to something correct, keeping strictness with the target. (Maybe add isn't the right namespace but just first thing that came to mind).

EDIT: another idea

uv init --with-timestamp --script foo.py

@manzt
Copy link
Contributor Author

manzt commented Oct 25, 2024

Ahh, seeing you already suggested a command in #6562... just catching up

@zanieb
Copy link
Member

zanieb commented Oct 25, 2024

Yeah I think this kind of needs to wait for a uv config set command

@BurntSushi
Copy link
Member

Yeah, I basically think that we probably should not support writing unadorned dates in a persistent configuration that is meant to be shared among folks that may be in different time zones. It just makes the date ambiguous. Even if we defaulted to UTC, it's still not immediately obvious to someone reading the config file that it would be UTC. And it seems much more natural to assume it isn't UTC but actually local, which would lead to folks reading the date incorrectly in some cases.

I think a hypothetical uv config set command that owns the "interpretation of the date as local, but writes it as an unambiguous timestamp" is a good idea.

There are other options for writing timestamps to config files that are maybe a little more user friendly. For example, we could support 2024-10-28T00:00:00-04[US/Eastern]. But probably just writing out 2024-10-28T00:00:00Z is the way to go.

@zanieb
Copy link
Member

zanieb commented Oct 28, 2024

Sounds like we should update the documentation to clarify this.

@manzt
Copy link
Contributor Author

manzt commented Oct 28, 2024

I think a hypothetical uv config set command that owns the "interpretation of the date as local, but writes it as an unambiguous timestamp" is a good idea.

Sounds good to me! Not something I'd considered and appreciate the thoughtfulness and explanation by you both.

@manzt
Copy link
Contributor Author

manzt commented Nov 13, 2024

Until a built-in command exists, I’ve added stamp command to juv for pinning unambiguous timestamps directly into inline script metadata (in both notebooks and scripts).

uv init --script foo.py
uv add --script foo.py polars
uvx juv stamp foo.py --date=2021-07-07

BurntSushi pushed a commit that referenced this pull request Jan 6, 2025
…9135)

<!--
Thank you for contributing to uv! To help us out with reviewing, please
consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->

## Summary

Follow up to #8553

Clarifies that the `exclude-newer` setting must be a full timestamp and
not a date.

<!-- What's the purpose of the change? What does it do, and why? -->

## Test Plan

N/A

<!-- How was it tested? -->
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.

3 participants