Some docker CI jobs failing, all due to

Error: missing API token, please run `depot login`
Error: failed with: Error: missing API token, please run `depot login`

I've checked other recent PRs and I'm seeing similar failures (example), so I suspect this is a transient issue not related to my changes.

This being my first PR in uv, I'm especially open to any and all feedback / changes requested (not that I wouldn't be if it was my 500th 🙂). If there's anything I can do to make the review process easier, don't hesitate to let me know!

Cool thanks!

Some quick high-level thoughts (I haven't looked at the code yet)

Passing --preview-features via CLI does NOT combine with uv.tool.preview-features, it overrides it completely. This behavior makes sense to me, otherwise there isn't a clear way to say "No, don't use your config feature X, use Y and Z instead".

I think we should actually combine them. If we get feedback that people need to be able to disable specific features, then the canonical way to do so in uv's interface would be to add a --no-preview-features <names> flag.

Don't worry about those Depot authentication failures — we're working with them to resolve those.

I think we should actually combine them. If we get feedback that people need to be able to disable specific features, then the canonical way to do so in uv's interface would be to add a --no-preview-features flag.

Got it, thanks for explaining. That should be an easy fix, I’ll get to that later today.

@zanieb I want to clarify one thing before submitting my revision. Suppose we pass --preview-features pylock with an underlying config like

preview = false
preview-features = ["format"]

1. Would the final set of features be format | pylock or would we prefer to take only pylock since uv.tool.preview = false?
2. Similarly, if tool.uv.preview = true, should any --preview-features merge into that de facto, resulting in all features being enabled?

I would intuitively expect that

1. We just take pylock when tool.uv.preview = false, ignoring the config features.
2. Extra --preview-features are swallowed by uv.tool.preview = true.

I pushed what made intuitive sense to me:

1. We just take pylock when tool.uv.preview = false, ignoring the config features.
2. Extra --preview-features are swallowed by uv.tool.preview = true.

Happy to change this if requested!

Nice, no more Depot auth failures.

I'm curious if anyone will get a chance to look at this PR? It would be nice to have this functionality at my current job. I'm sure it's very low-priority, though, so no worries.

If I made any mistakes in my approach that makes this work too time-consuming to review, I'd be happy to take that feedback so I can contribute more productively to uv in the future 🙂

Suppose we pass --preview-features pylock with an underlying config like [...]

I think if we see a config file like

preview = false
preview-features = ["format"]

we should probably just fail?

Similarly, if tool.uv.preview = true, should any --preview-features merge into that de facto, resulting in all features being enabled?

I think I'd expect us just to activate the subset of preview features specified in the CLI since the CLI takes precedence over the configuration file, though I'm not entirely sure and don't feel strongly.

Sorry for the delayed review, we've just had a lot on our backlog — it's not anything you did wrong.

I think if we see a config file like

preview = false
preview-features = ["format"]

we should probably just fail?

That makes intuitive sense to me, but doesn't match the current behavior of the commandline arguments, where you can pass --no-preview --preview-features upgrade-python without complaint.

Unless I'm missing something (very possible), it seems like implementing the validation you suggest is nontrivial. The closest thing I can find is validate_uv_toml, seems like we'd have to add an entirely new validation flow. Does that sound right?

Does it make sense to add that validation in this PR or in a separate effort to keep the scope here limited? I'm open to either option.

I think we can be more strict in the configuration file than in the CLI.

The closest thing I can find is validate_uv_toml, seems like we'd have to add an entirely new validation flow.

Ah, hm. That's a little unfortunate. It does seem useful long-term though.

Does it make sense to add that validation in this PR or in a separate effort to keep the scope here limited? I'm open to either option.

I think the only trouble with doing it in a separate pull request is that we don't want to release support for including both fields then "break" it in a subsequent release. Would you mind sketching it in a separate pull request and we can merge them together?

Would you mind sketching it in a separate pull request and we can merge them together?

Sure thing! I'll try to get to that this weekend.

Merging this PR will not alter performance

✅ 5 untouched benchmarks

_{Comparing j-helland:jwh/toml-config-preview-features (44d7749) with main (deb394c)}

@zanieb I sketched what you suggested in #17202 let me know what you think.

Sorry I lost track of this over the holidays! Will revisit.

It hurt my soul to do a merge for this but hey, the precedent was set 🤷 .

I've fixed up the conflicts with the Preview changes, as a bonus the code is cleaner and doesn't do runtime type checking anymore. I'll rebase the other PR onto this one once this one passes CI.

Work has been busy lately and I lost track of this myself!

Thanks @EliteTK for fixing this up, I really like your simplifications with the PreviewFeature enum.

I believe what remains here is deciding how to proceed with #17202 (259a9d4), specifically #17202 (comment). Happy to continue working on it if you all have feedback!

There's a potential issue here that now if I have a local pyproject.toml with preview-features = ["foo", "bar"] and a user uv.toml with preview = false then after combining (even with the validation patch AIUI) you will get an inconsistent preview = false ; preview-features = ["foo", "bar"] config when in reality you want the project config to override the user level one.

But that could be fixed by having Options just hold a Option<Preview> I think, which would also require the changes suggested in #17202 (comment) .

That being said, I am wondering if there's justification to have preview take a bool or a table instead. e.g.

preview = false
preview = true
preview = ["foo", "bar", "baz"]
# Or if that's not clear, then alternatively:
preview = { features = ["foo", "bar", "baz"] }

It kind of breaks with the established convention we've had going in this regard with --preview and --preview-features. I don't personally like denormalised data when it can be avoided though. Maybe @zanieb can chip in their opinion on this specific design aspect.

It would mean that Preview can implement its own Deserialize and then the validation can sit there without needing to add a Deserialize impl for Options. But that route is not particularly bad either.

There's a potential issue here that now if I have a local pyproject.toml with preview-features = ["foo", "bar"] and a user uv.toml with preview = false then after combining (even with the validation patch AIUI) you will get an inconsistent preview = false ; preview-features = ["foo", "bar"] config when in reality you want the project config to override the user level one.

That's true, that should be fixed.

I agree that denormalized data leads to clunkier code here. I'd personally be fine with

# boolean values
preview = true
preview = false
preview = { features = ["foo", "bar", "baz"] }

as a compromise between the implementation's elegance and the clarity of having config fields correspond one-to-one with commandline args per the original design.

I'll defer to you all about whether the convention is worth breaking, though. I could see a reasonable argument for the clarity of the UI taking priority here.

We discussed this internally, there's a few other options which follow the same duplicate pattern, so in that regard, we are going to keep preview and preview-features here for now. But what we can do is this:

Keep preview = bool for now. But hide it.

Add preview-features = bool | list (to implement this, just add an enum with serde(untagged) on it to OptionsWire), document the behaviour.

Use Preview directly in the Options struct. Do the validation (as suggested in the other PR) by replacing impl From<OptionsWire> for Options with impl Deserialize for Options. All that so that we use the default logic for how we do combining between different files - for consistency.