From e4de538daef6026f5ff4022abe0abc5b5109a87d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 21 Aug 2025 19:35:07 +0100 Subject: [PATCH] Remove `UpgradeSelection` struct (#15422) ## Summary After #15395, I realized that we didn't actually need a separate struct for this since we now pass it around as an `Option`. (The key change from #15395 is that when combining, we treat the options as a single unit.) --- crates/uv-cli/src/options.rs | 6 +- .../uv-configuration/src/package_options.rs | 116 ++++++------------ crates/uv-settings/src/combine.rs | 4 +- crates/uv-settings/src/settings.rs | 10 +- crates/uv/src/commands/tool/install.rs | 5 +- crates/uv/src/settings.rs | 41 +++---- 6 files changed, 66 insertions(+), 116 deletions(-) diff --git a/crates/uv-cli/src/options.rs b/crates/uv-cli/src/options.rs index 492942f1b..c9b076a24 100644 --- a/crates/uv-cli/src/options.rs +++ b/crates/uv-cli/src/options.rs @@ -1,7 +1,7 @@ use anstream::eprintln; use uv_cache::Refresh; -use uv_configuration::UpgradeSelection; +use uv_configuration::Upgrade; use uv_distribution_types::{ConfigSettings, PackageConfigSettings, Requirement}; use uv_resolver::{ExcludeNewer, ExcludeNewerPackage, PrereleaseMode}; use uv_settings::{Combine, PipOptions, ResolverInstallerOptions, ResolverOptions}; @@ -334,7 +334,7 @@ pub fn resolver_options( .filter_map(Maybe::into_option) .collect() }), - upgrade: UpgradeSelection::from_args( + upgrade: Upgrade::from_args( flag(upgrade, no_upgrade, "no-upgrade"), upgrade_package.into_iter().map(Requirement::from).collect(), ), @@ -445,7 +445,7 @@ pub fn resolver_installer_options( .filter_map(Maybe::into_option) .collect() }), - upgrade: UpgradeSelection::from_args( + upgrade: Upgrade::from_args( flag(upgrade, no_upgrade, "upgrade"), upgrade_package.into_iter().map(Requirement::from).collect(), ), diff --git a/crates/uv-configuration/src/package_options.rs b/crates/uv-configuration/src/package_options.rs index f4b04a245..a22e1523d 100644 --- a/crates/uv-configuration/src/package_options.rs +++ b/crates/uv-configuration/src/package_options.rs @@ -134,55 +134,6 @@ impl From for Refresh { } } -/// An upgrade selection as specified by a user on the command line or in a configuration file. -#[derive(Debug, Default, Clone)] -pub enum UpgradeSelection { - /// Prefer pinned versions from the existing lockfile, if possible. - #[default] - None, - - /// Allow package upgrades for all packages, ignoring the existing lockfile. - All, - - /// Allow package upgrades, but only for the specified packages. - Packages(Vec), -} - -impl UpgradeSelection { - /// Determine the upgrade selection strategy from the command-line arguments. - pub fn from_args(upgrade: Option, upgrade_package: Vec) -> Option { - match upgrade { - Some(true) => Some(Self::All), - // TODO(charlie): `--no-upgrade` with `--upgrade-package` should allow the specified - // packages to be upgraded. Right now, `--upgrade-package` is silently ignored. - Some(false) => Some(Self::None), - None if upgrade_package.is_empty() => None, - None => Some(Self::Packages(upgrade_package)), - } - } - - /// Combine a set of [`UpgradeSelection`] values. - #[must_use] - pub fn combine(self, other: Self) -> Self { - match self { - // Setting `--upgrade` or `--no-upgrade` should clear previous `--upgrade-package` selections. - Self::All | Self::None => self, - Self::Packages(self_packages) => match other { - // If `--upgrade` was enabled previously, `--upgrade-package` is subsumed by upgrading all packages. - Self::All => other, - // If `--no-upgrade` was enabled previously, then `--upgrade-package` enables an explicit upgrade of those packages. - Self::None => Self::Packages(self_packages), - // If `--upgrade-package` was included twice, combine the requirements. - Self::Packages(other_packages) => { - let mut combined = self_packages; - combined.extend(other_packages); - Self::Packages(combined) - } - }, - } - } -} - /// Whether to allow package upgrades. #[derive(Debug, Default, Clone)] pub enum Upgrade { @@ -197,28 +148,27 @@ pub enum Upgrade { Packages(FxHashMap>), } -/// Determine the [`Upgrade`] strategy from the command-line arguments. -impl From> for Upgrade { - fn from(value: Option) -> Self { - match value { - None => Self::None, - Some(UpgradeSelection::None) => Self::None, - Some(UpgradeSelection::All) => Self::All, - Some(UpgradeSelection::Packages(requirements)) => Self::Packages( - requirements - .into_iter() - .fold(FxHashMap::default(), |mut map, requirement| { - map.entry(requirement.name.clone()) - .or_default() - .push(requirement); - map - }), - ), +impl Upgrade { + /// Determine the upgrade selection strategy from the command-line arguments. + pub fn from_args(upgrade: Option, upgrade_package: Vec) -> Option { + match upgrade { + Some(true) => Some(Self::All), + // TODO(charlie): `--no-upgrade` with `--upgrade-package` should allow the specified + // packages to be upgraded. Right now, `--upgrade-package` is silently ignored. + Some(false) => Some(Self::None), + None if upgrade_package.is_empty() => None, + None => Some(Self::Packages(upgrade_package.into_iter().fold( + FxHashMap::default(), + |mut map, requirement| { + map.entry(requirement.name.clone()) + .or_default() + .push(requirement); + map + }, + ))), } } -} -impl Upgrade { /// Create an [`Upgrade`] strategy to upgrade a single package. pub fn package(package_name: PackageName) -> Self { Self::Packages({ @@ -265,19 +215,23 @@ impl Upgrade { /// Combine a set of [`Upgrade`] values. #[must_use] pub fn combine(self, other: Self) -> Self { - match (self, other) { - // If both are `None`, the result is `None`. - (Self::None, Self::None) => Self::None, - // If either is `All`, the result is `All`. - (Self::All, _) | (_, Self::All) => Self::All, - // If one is `None`, the result is the other. - (Self::Packages(a), Self::None) => Self::Packages(a), - (Self::None, Self::Packages(b)) => Self::Packages(b), - // If both are `Packages`, the result is the union of the two. - (Self::Packages(mut a), Self::Packages(b)) => { - a.extend(b); - Self::Packages(a) - } + match self { + // Setting `--upgrade` or `--no-upgrade` should clear previous `--upgrade-package` selections. + Self::All | Self::None => self, + Self::Packages(self_packages) => match other { + // If `--upgrade` was enabled previously, `--upgrade-package` is subsumed by upgrading all packages. + Self::All => other, + // If `--no-upgrade` was enabled previously, then `--upgrade-package` enables an explicit upgrade of those packages. + Self::None => Self::Packages(self_packages), + // If `--upgrade-package` was included twice, combine the requirements. + Self::Packages(other_packages) => { + let mut combined = self_packages; + for (package, requirements) in other_packages { + combined.entry(package).or_default().extend(requirements); + } + Self::Packages(combined) + } + }, } } } diff --git a/crates/uv-settings/src/combine.rs b/crates/uv-settings/src/combine.rs index 02be876ec..7021f9db3 100644 --- a/crates/uv-settings/src/combine.rs +++ b/crates/uv-settings/src/combine.rs @@ -5,7 +5,7 @@ use url::Url; use uv_configuration::{ ExportFormat, IndexStrategy, KeyringProviderType, RequiredVersion, TargetTriple, - TrustedPublishing, UpgradeSelection, + TrustedPublishing, Upgrade, }; use uv_distribution_types::{ ConfigSettings, ExtraBuildVariables, Index, IndexUrl, PackageConfigSettings, PipExtraIndex, @@ -181,7 +181,7 @@ impl Combine for Option { } } -impl Combine for Option { +impl Combine for Option { fn combine(self, other: Self) -> Self { match (self, other) { (Some(a), Some(b)) => Some(a.combine(b)), diff --git a/crates/uv-settings/src/settings.rs b/crates/uv-settings/src/settings.rs index 96509f524..9185eedd8 100644 --- a/crates/uv-settings/src/settings.rs +++ b/crates/uv-settings/src/settings.rs @@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize}; use uv_cache_info::CacheKey; use uv_configuration::{ IndexStrategy, KeyringProviderType, PackageNameSpecifier, RequiredVersion, TargetTriple, - TrustedHost, TrustedPublishing, UpgradeSelection, + TrustedHost, TrustedPublishing, Upgrade, }; use uv_distribution_types::{ ConfigSettings, ExtraBuildVariables, Index, IndexUrl, IndexUrlError, PackageConfigSettings, @@ -369,7 +369,7 @@ pub struct ResolverOptions { pub config_settings_package: Option, pub exclude_newer: ExcludeNewer, pub link_mode: Option, - pub upgrade: Option, + pub upgrade: Option, pub no_build: Option, pub no_build_package: Option>, pub no_binary: Option, @@ -407,7 +407,7 @@ pub struct ResolverInstallerOptions { pub link_mode: Option, pub compile_bytecode: Option, pub no_sources: Option, - pub upgrade: Option, + pub upgrade: Option, pub reinstall: Option, pub reinstall_package: Option>, pub no_build: Option, @@ -473,7 +473,7 @@ impl From for ResolverInstallerOptions { link_mode, compile_bytecode, no_sources, - upgrade: UpgradeSelection::from_args( + upgrade: Upgrade::from_args( upgrade, upgrade_package .into_iter() @@ -1886,7 +1886,7 @@ impl From for ResolverOptions { .collect(), ), link_mode: value.link_mode, - upgrade: UpgradeSelection::from_args( + upgrade: Upgrade::from_args( value.upgrade, value .upgrade_package diff --git a/crates/uv/src/commands/tool/install.rs b/crates/uv/src/commands/tool/install.rs index b191d0e1e..8b0b9070a 100644 --- a/crates/uv/src/commands/tool/install.rs +++ b/crates/uv/src/commands/tool/install.rs @@ -234,10 +234,7 @@ pub(crate) async fn install( let settings = if request.is_latest() { ResolverInstallerSettings { resolver: ResolverSettings { - upgrade: settings - .resolver - .upgrade - .combine(Upgrade::package(package_name.clone())), + upgrade: Upgrade::package(package_name.clone()).combine(settings.resolver.upgrade), ..settings.resolver }, ..settings diff --git a/crates/uv/src/settings.rs b/crates/uv/src/settings.rs index 27793c60d..6a8dcf827 100644 --- a/crates/uv/src/settings.rs +++ b/crates/uv/src/settings.rs @@ -24,7 +24,7 @@ use uv_configuration::{ BuildOptions, Concurrency, DependencyGroups, DryRun, EditableMode, ExportFormat, ExtrasSpecification, HashCheckingMode, IndexStrategy, InstallOptions, KeyringProviderType, NoBinary, NoBuild, Preview, ProjectBuildBackend, Reinstall, RequiredVersion, SourceStrategy, - TargetTriple, TrustedHost, TrustedPublishing, Upgrade, UpgradeSelection, VersionControlSystem, + TargetTriple, TrustedHost, TrustedPublishing, Upgrade, VersionControlSystem, }; use uv_distribution_types::{ ConfigSettings, DependencyMetadata, ExtraBuildVariables, Index, IndexLocations, IndexUrl, @@ -2852,7 +2852,7 @@ impl From for ResolverSettings { exclude_newer: value.exclude_newer, link_mode: value.link_mode.unwrap_or_default(), sources: SourceStrategy::from_args(value.no_sources.unwrap_or_default()), - upgrade: Upgrade::from(value.upgrade), + upgrade: value.upgrade.unwrap_or_default(), build_options: BuildOptions::new( NoBinary::from_args(value.no_binary, value.no_binary_package.unwrap_or_default()), NoBuild::from_args(value.no_build, value.no_build_package.unwrap_or_default()), @@ -2943,7 +2943,7 @@ impl From for ResolverInstallerSettings { prerelease: value.prerelease.unwrap_or_default(), resolution: value.resolution.unwrap_or_default(), sources: SourceStrategy::from_args(value.no_sources.unwrap_or_default()), - upgrade: Upgrade::from(value.upgrade), + upgrade: value.upgrade.unwrap_or_default(), }, compile_bytecode: value.compile_bytecode.unwrap_or_default(), reinstall: Reinstall::from_args( @@ -3317,24 +3317,23 @@ impl PipSettings { args.no_sources.combine(no_sources).unwrap_or_default(), ), strict: args.strict.combine(strict).unwrap_or_default(), - upgrade: Upgrade::from( - UpgradeSelection::from_args( - args.upgrade, - args.upgrade_package - .into_iter() - .flatten() - .map(Requirement::from) - .collect(), - ) - .combine(UpgradeSelection::from_args( - upgrade, - upgrade_package - .into_iter() - .flatten() - .map(Requirement::from) - .collect(), - )), - ), + upgrade: Upgrade::from_args( + args.upgrade, + args.upgrade_package + .into_iter() + .flatten() + .map(Requirement::from) + .collect(), + ) + .combine(Upgrade::from_args( + upgrade, + upgrade_package + .into_iter() + .flatten() + .map(Requirement::from) + .collect(), + )) + .unwrap_or_default(), reinstall: Reinstall::from_args( args.reinstall.combine(reinstall), args.reinstall_package