-
Notifications
You must be signed in to change notification settings - Fork 413
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
Add persistent configuration for non-pip
APIs
#4294
Conversation
b9557c0
to
c3ac307
Compare
c3ac307
to
4df0f77
Compare
59b09d6
to
486ab2f
Compare
// However, the user-level `tool.uv.pip` settings override the project-level `tool.uv` settings. | ||
// This is awkward, but we merge the user configuration into the workspace configuration, so | ||
// the resulting configuration has both `tool.uv.pip.resolution` (from the user configuration) | ||
// and `tool.uv.resolution` (from the workspace settings), so we choose the former. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really ideal, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little awkward but at least it's consistent, maybe we should warn in that case? Are we going to have more complex merging rules for the pip namespace from the user-level config?
486ab2f
to
cfb022a
Compare
let client_builder = BaseClientBuilder::new() | ||
.connectivity(connectivity) | ||
.native_tls(native_tls) | ||
.keyring(*keyring_provider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate, but I wonder if we should have a Settings::as_client_builder
or BaseClientBuilder::from_settings
let client = RegistryClientBuilder::new(cache.clone()) | ||
.native_tls(native_tls) | ||
.connectivity(connectivity) | ||
.index_urls(index_locations.index_urls()) | ||
.index_strategy(*index_strategy) | ||
.keyring(*keyring_provider) | ||
.markers(markers) | ||
.platform(venv.interpreter().platform()) | ||
.platform(interpreter.platform()) | ||
.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so it's on your mind as well as mine, I feel like RegistryClientBuilder
should take a BaseClientBuilder
instead of duplicating the global options? I found this awkward in some places where I needed to construct both types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I can look at it at some point.
@zanieb - Are you cool with this modulo (1) renaming, and (2) removing the |
I think so! It seems pretty reasonable. |
Add Wire up Split into multiple arguments
3e83554
to
99413a4
Compare
Okay, I believe I addressed the biggest feedback. I'm saddened by how much boilerplate will be required to add a new setting in the future. Perhaps we can DRY a lot of this up with macros in the future, but it's not my strength. |
99413a4
to
16de0ce
Compare
16de0ce
to
164b8e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Summary
This PR introduces top-level configuration for uv, such that you can do:
And
uv pip compile
,uv run
,uv tool run
, etc., will all respect that configuration.The settings that were escalated to the top-level remain on
tool.uv.pip
too, but they're only respected inuv pip
commands. If they're specified in both places, then thepip
settings win out.While making this change, I also wired up some of the global options, like
connectivity
andnative_tls
, through to all the relevant places.Closes #4250.