From d75d6d7c7c02766b46fbe51dcdc20f2849d5619a Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Sun, 15 Jan 2023 10:28:00 +0100 Subject: [PATCH] refactor: Split CliSettings from Settings We want to automatically derive Hash for the library settings, which requires us to split off all the settings unused by the library (since these shouldn't affect the hash used by ruff_cli::cache). --- ruff_cli/src/cache.rs | 16 +++++++---- ruff_cli/src/commands.rs | 14 +++++----- ruff_cli/src/diagnostics.rs | 10 +++---- ruff_cli/src/main.rs | 41 +++++++++++++-------------- src/lib_native.rs | 4 +-- src/resolver.rs | 36 ++++++++++++++---------- src/settings/mod.rs | 55 +++++++++++++++++++++++-------------- 7 files changed, 100 insertions(+), 76 deletions(-) diff --git a/ruff_cli/src/cache.rs b/ruff_cli/src/cache.rs index e23d47fd52..1f2188b688 100644 --- a/ruff_cli/src/cache.rs +++ b/ruff_cli/src/cache.rs @@ -9,7 +9,7 @@ use filetime::FileTime; use log::error; use path_absolutize::Absolutize; use ruff::message::Message; -use ruff::settings::{flags, Settings}; +use ruff::settings::{flags, AllSettings, Settings}; use serde::{Deserialize, Serialize}; const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); @@ -80,10 +80,14 @@ fn read_sync(cache_dir: &Path, key: u64) -> Result, std::io::Error> { pub fn get>( path: P, metadata: &fs::Metadata, - settings: &Settings, + settings: &AllSettings, autofix: flags::Autofix, ) -> Option> { - let encoded = read_sync(&settings.cache_dir, cache_key(path, settings, autofix)).ok()?; + let encoded = read_sync( + &settings.cli.cache_dir, + cache_key(path, &settings.lib, autofix), + ) + .ok()?; let (mtime, messages) = match bincode::deserialize::(&encoded[..]) { Ok(CheckResult { metadata: CacheMetadata { mtime }, @@ -104,7 +108,7 @@ pub fn get>( pub fn set>( path: P, metadata: &fs::Metadata, - settings: &Settings, + settings: &AllSettings, autofix: flags::Autofix, messages: &[Message], ) { @@ -115,8 +119,8 @@ pub fn set>( messages, }; if let Err(e) = write_sync( - &settings.cache_dir, - cache_key(path, settings, autofix), + &settings.cli.cache_dir, + cache_key(path, &settings.lib, autofix), &bincode::serialize(&check_result).unwrap(), ) { error!("Failed to write to cache: {e:?}"); diff --git a/ruff_cli/src/commands.rs b/ruff_cli/src/commands.rs index 886dce15e2..5509cce87b 100644 --- a/ruff_cli/src/commands.rs +++ b/ruff_cli/src/commands.rs @@ -56,19 +56,19 @@ pub fn run( if matches!(cache, flags::Cache::Enabled) { match &pyproject_strategy { PyprojectDiscovery::Fixed(settings) => { - if let Err(e) = cache::init(&settings.cache_dir) { + if let Err(e) = cache::init(&settings.cli.cache_dir) { error!( "Failed to initialize cache at {}: {e:?}", - settings.cache_dir.to_string_lossy() + settings.cli.cache_dir.to_string_lossy() ); } } PyprojectDiscovery::Hierarchical(default) => { for settings in std::iter::once(default).chain(resolver.iter()) { - if let Err(e) = cache::init(&settings.cache_dir) { + if let Err(e) = cache::init(&settings.cli.cache_dir) { error!( "Failed to initialize cache at {}: {e:?}", - settings.cache_dir.to_string_lossy() + settings.cli.cache_dir.to_string_lossy() ); } } @@ -97,7 +97,7 @@ pub fn run( .parent() .and_then(|parent| package_roots.get(parent)) .and_then(|package| *package); - let settings = resolver.resolve(path, pyproject_strategy); + let settings = resolver.resolve_all(path, pyproject_strategy); lint_path(path, package, settings, cache, autofix) .map_err(|e| (Some(path.to_owned()), e.to_string())) } @@ -171,9 +171,9 @@ pub fn run_stdin( }; let package_root = filename .and_then(Path::parent) - .and_then(|path| packaging::detect_package_root(path, &settings.namespace_packages)); + .and_then(|path| packaging::detect_package_root(path, &settings.lib.namespace_packages)); let stdin = read_from_stdin()?; - let mut diagnostics = lint_stdin(filename, package_root, &stdin, settings, autofix)?; + let mut diagnostics = lint_stdin(filename, package_root, &stdin, &settings.lib, autofix)?; diagnostics.messages.sort_unstable(); Ok(diagnostics) } diff --git a/ruff_cli/src/diagnostics.rs b/ruff_cli/src/diagnostics.rs index 35edfec9a9..7d9fe055a0 100644 --- a/ruff_cli/src/diagnostics.rs +++ b/ruff_cli/src/diagnostics.rs @@ -9,7 +9,7 @@ use anyhow::Result; use log::debug; use ruff::linter::{lint_fix, lint_only}; use ruff::message::Message; -use ruff::settings::{flags, Settings}; +use ruff::settings::{flags, AllSettings, Settings}; use ruff::{fix, fs}; use similar::TextDiff; @@ -38,12 +38,12 @@ impl AddAssign for Diagnostics { pub fn lint_path( path: &Path, package: Option<&Path>, - settings: &Settings, + settings: &AllSettings, cache: flags::Cache, autofix: fix::FixMode, ) -> Result { // Validate the `Settings` and return any errors. - settings.validate()?; + settings.lib.validate()?; // Check the cache. // TODO(charlie): `fixer::Mode::Apply` and `fixer::Mode::Diff` both have @@ -69,7 +69,7 @@ pub fn lint_path( // Lint the file. let (messages, fixed) = if matches!(autofix, fix::FixMode::Apply | fix::FixMode::Diff) { - let (transformed, fixed, messages) = lint_fix(&contents, path, package, settings)?; + let (transformed, fixed, messages) = lint_fix(&contents, path, package, &settings.lib)?; if fixed > 0 { if matches!(autofix, fix::FixMode::Apply) { write(path, transformed)?; @@ -85,7 +85,7 @@ pub fn lint_path( } (messages, fixed) } else { - let messages = lint_only(&contents, path, package, settings, autofix.into())?; + let messages = lint_only(&contents, path, package, &settings.lib, autofix.into())?; let fixed = 0; (messages, fixed) }; diff --git a/ruff_cli/src/main.rs b/ruff_cli/src/main.rs index 50d2823c7b..2557da49ab 100644 --- a/ruff_cli/src/main.rs +++ b/ruff_cli/src/main.rs @@ -16,8 +16,8 @@ use ::ruff::resolver::{ resolve_settings_with_processor, ConfigProcessor, FileDiscovery, PyprojectDiscovery, Relativity, }; use ::ruff::settings::configuration::Configuration; +use ::ruff::settings::pyproject; use ::ruff::settings::types::SerializationFormat; -use ::ruff::settings::{pyproject, Settings}; use ::ruff::{fix, fs, warn_user_once}; use anyhow::Result; use clap::{CommandFactory, Parser}; @@ -26,6 +26,7 @@ use colored::Colorize; use notify::{recommended_watcher, RecursiveMode, Watcher}; use path_absolutize::path_dedot; use printer::{Printer, Violations}; +use ruff::settings::{AllSettings, CliSettings}; mod cache; mod cli; @@ -48,7 +49,7 @@ fn resolve( // First priority: if we're running in isolated mode, use the default settings. let mut config = Configuration::default(); overrides.process_config(&mut config); - let settings = Settings::from_configuration(config, &path_dedot::CWD)?; + let settings = AllSettings::from_configuration(config, &path_dedot::CWD)?; Ok(PyprojectDiscovery::Fixed(settings)) } else if let Some(pyproject) = config { // Second priority: the user specified a `pyproject.toml` file. Use that @@ -82,7 +83,7 @@ fn resolve( // as the "default" settings.) let mut config = Configuration::default(); overrides.process_config(&mut config); - let settings = Settings::from_configuration(config, &path_dedot::CWD)?; + let settings = AllSettings::from_configuration(config, &path_dedot::CWD)?; Ok(PyprojectDiscovery::Hierarchical(settings)) } } @@ -113,35 +114,31 @@ pub fn main() -> Result { // Validate the `Settings` and return any errors. match &pyproject_strategy { - PyprojectDiscovery::Fixed(settings) => settings.validate()?, - PyprojectDiscovery::Hierarchical(settings) => settings.validate()?, + PyprojectDiscovery::Fixed(settings) => settings.lib.validate()?, + PyprojectDiscovery::Hierarchical(settings) => settings.lib.validate()?, }; // Extract options that are included in `Settings`, but only apply at the top // level. let file_strategy = FileDiscovery { force_exclude: match &pyproject_strategy { - PyprojectDiscovery::Fixed(settings) => settings.force_exclude, - PyprojectDiscovery::Hierarchical(settings) => settings.force_exclude, + PyprojectDiscovery::Fixed(settings) => settings.lib.force_exclude, + PyprojectDiscovery::Hierarchical(settings) => settings.lib.force_exclude, }, respect_gitignore: match &pyproject_strategy { - PyprojectDiscovery::Fixed(settings) => settings.respect_gitignore, - PyprojectDiscovery::Hierarchical(settings) => settings.respect_gitignore, + PyprojectDiscovery::Fixed(settings) => settings.lib.respect_gitignore, + PyprojectDiscovery::Hierarchical(settings) => settings.lib.respect_gitignore, }, }; - let (fix, fix_only, format, update_check) = match &pyproject_strategy { - PyprojectDiscovery::Fixed(settings) => ( - settings.fix, - settings.fix_only, - settings.format, - settings.update_check, - ), - PyprojectDiscovery::Hierarchical(settings) => ( - settings.fix, - settings.fix_only, - settings.format, - settings.update_check, - ), + let CliSettings { + fix, + fix_only, + format, + update_check, + .. + } = match &pyproject_strategy { + PyprojectDiscovery::Fixed(settings) => settings.cli.clone(), + PyprojectDiscovery::Hierarchical(settings) => settings.cli.clone(), }; if let Some(code) = cli.explain { diff --git a/src/lib_native.rs b/src/lib_native.rs index c296f219cc..eea1fcb96d 100644 --- a/src/lib_native.rs +++ b/src/lib_native.rs @@ -17,10 +17,10 @@ use crate::{directives, packaging, resolver}; fn resolve(path: &Path) -> Result { if let Some(pyproject) = pyproject::find_settings_toml(path)? { // First priority: `pyproject.toml` in the current `Path`. - resolver::resolve_settings(&pyproject, &Relativity::Parent) + Ok(resolver::resolve_settings(&pyproject, &Relativity::Parent)?.lib) } else if let Some(pyproject) = pyproject::find_user_settings_toml() { // Second priority: user-specific `pyproject.toml`. - resolver::resolve_settings(&pyproject, &Relativity::Cwd) + Ok(resolver::resolve_settings(&pyproject, &Relativity::Cwd)?.lib) } else { // Fallback: default settings. Settings::from_configuration(Configuration::default(), &path_dedot::CWD) diff --git a/src/resolver.rs b/src/resolver.rs index 0d47108dc4..3e83cb6e56 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -14,7 +14,7 @@ use rustc_hash::FxHashSet; use crate::fs; use crate::settings::configuration::Configuration; use crate::settings::pyproject::settings_toml; -use crate::settings::{pyproject, Settings}; +use crate::settings::{pyproject, AllSettings, Settings}; /// The strategy used to discover Python files in the filesystem.. #[derive(Debug)] @@ -29,10 +29,10 @@ pub struct FileDiscovery { pub enum PyprojectDiscovery { /// Use a fixed `pyproject.toml` file for all Python files (i.e., one /// provided on the command-line). - Fixed(Settings), + Fixed(AllSettings), /// Use the closest `pyproject.toml` file in the filesystem hierarchy, or /// the default settings. - Hierarchical(Settings), + Hierarchical(AllSettings), } /// The strategy for resolving file paths in a `pyproject.toml`. @@ -58,17 +58,21 @@ impl Relativity { #[derive(Default)] pub struct Resolver { - settings: BTreeMap, + settings: BTreeMap, } impl Resolver { /// Add a resolved `Settings` under a given `PathBuf` scope. - pub fn add(&mut self, path: PathBuf, settings: Settings) { + pub fn add(&mut self, path: PathBuf, settings: AllSettings) { self.settings.insert(path, settings); } - /// Return the appropriate `Settings` for a given `Path`. - pub fn resolve<'a>(&'a self, path: &Path, strategy: &'a PyprojectDiscovery) -> &'a Settings { + /// Return the appropriate `AllSettings` for a given `Path`. + pub fn resolve_all<'a>( + &'a self, + path: &Path, + strategy: &'a PyprojectDiscovery, + ) -> &'a AllSettings { match strategy { PyprojectDiscovery::Fixed(settings) => settings, PyprojectDiscovery::Hierarchical(default) => self @@ -86,8 +90,12 @@ impl Resolver { } } + pub fn resolve<'a>(&'a self, path: &Path, strategy: &'a PyprojectDiscovery) -> &'a Settings { + &self.resolve_all(path, strategy).lib + } + /// Return an iterator over the resolved `Settings` in this `Resolver`. - pub fn iter(&self) -> impl Iterator { + pub fn iter(&self) -> impl Iterator { self.settings.values() } @@ -100,11 +108,11 @@ impl Resolver { // `Settings` for each path, but that's more expensive. match &strategy { PyprojectDiscovery::Fixed(settings) => { - settings.validate()?; + settings.lib.validate()?; } PyprojectDiscovery::Hierarchical(default) => { for settings in std::iter::once(default).chain(self.iter()) { - settings.validate()?; + settings.lib.validate()?; } } } @@ -176,15 +184,15 @@ pub fn resolve_scoped_settings( pyproject: &Path, relativity: &Relativity, processor: impl ConfigProcessor, -) -> Result<(PathBuf, Settings)> { +) -> Result<(PathBuf, AllSettings)> { let project_root = relativity.resolve(pyproject); let configuration = resolve_configuration(pyproject, relativity, processor)?; - let settings = Settings::from_configuration(configuration, &project_root)?; + let settings = AllSettings::from_configuration(configuration, &project_root)?; Ok((project_root, settings)) } /// Extract the `Settings` from a given `pyproject.toml`. -pub fn resolve_settings(pyproject: &Path, relativity: &Relativity) -> Result { +pub fn resolve_settings(pyproject: &Path, relativity: &Relativity) -> Result { let (_project_root, settings) = resolve_scoped_settings(pyproject, relativity, &NoOpProcessor)?; Ok(settings) } @@ -195,7 +203,7 @@ pub fn resolve_settings_with_processor( pyproject: &Path, relativity: &Relativity, processor: impl ConfigProcessor, -) -> Result { +) -> Result { let (_project_root, settings) = resolve_scoped_settings(pyproject, relativity, processor)?; Ok(settings) } diff --git a/src/settings/mod.rs b/src/settings/mod.rs index 453a0028af..680b4cbb64 100644 --- a/src/settings/mod.rs +++ b/src/settings/mod.rs @@ -39,22 +39,53 @@ pub mod types; const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION"); +#[derive(Debug)] +pub struct AllSettings { + pub cli: CliSettings, + pub lib: Settings, +} + +impl AllSettings { + pub fn from_configuration(config: Configuration, project_root: &Path) -> Result { + Ok(Self { + cli: CliSettings { + cache_dir: config + .cache_dir + .clone() + .unwrap_or_else(|| cache_dir(project_root)), + fix: config.fix.unwrap_or(false), + fix_only: config.fix_only.unwrap_or(false), + format: config.format.unwrap_or_default(), + update_check: config.update_check.unwrap_or_default(), + }, + lib: Settings::from_configuration(config, project_root)?, + }) + } +} + +#[derive(Debug, Default, Clone)] +/// Settings that are not used by this library and +/// only here so that `ruff_cli` can use them. +pub struct CliSettings { + pub cache_dir: PathBuf, + pub fix: bool, + pub fix_only: bool, + pub format: SerializationFormat, + pub update_check: bool, +} + #[derive(Debug)] #[allow(clippy::struct_excessive_bools)] pub struct Settings { pub allowed_confusables: FxHashSet, pub builtins: Vec, - pub cache_dir: PathBuf, pub dummy_variable_rgx: Regex, pub enabled: FxHashSet, pub exclude: GlobSet, pub extend_exclude: GlobSet, pub external: FxHashSet, - pub fix: bool, - pub fix_only: bool, pub fixable: FxHashSet, pub force_exclude: bool, - pub format: SerializationFormat, pub ignore_init_module_imports: bool, pub line_length: usize, pub namespace_packages: Vec, @@ -66,7 +97,6 @@ pub struct Settings { pub target_version: PythonVersion, pub task_tags: Vec, pub typing_modules: Vec, - pub update_check: bool, // Plugins pub flake8_annotations: flake8_annotations::settings::Settings, pub flake8_bandit: flake8_bandit::settings::Settings, @@ -120,7 +150,6 @@ impl Settings { .map(FxHashSet::from_iter) .unwrap_or_default(), builtins: config.builtins.unwrap_or_default(), - cache_dir: config.cache_dir.unwrap_or_else(|| cache_dir(project_root)), dummy_variable_rgx: config .dummy_variable_rgx .unwrap_or_else(|| DEFAULT_DUMMY_VARIABLE_RGX.clone()), @@ -159,8 +188,6 @@ impl Settings { exclude: resolve_globset(config.exclude.unwrap_or_else(|| DEFAULT_EXCLUDE.clone()))?, extend_exclude: resolve_globset(config.extend_exclude)?, external: FxHashSet::from_iter(config.external.unwrap_or_default()), - fix: config.fix.unwrap_or(false), - fix_only: config.fix_only.unwrap_or(false), fixable: resolve_codes( [RuleCodeSpec { select: &config.fixable.unwrap_or_else(|| CATEGORIES.to_vec()), @@ -168,7 +195,6 @@ impl Settings { }] .into_iter(), ), - format: config.format.unwrap_or_default(), force_exclude: config.force_exclude.unwrap_or(false), ignore_init_module_imports: config.ignore_init_module_imports.unwrap_or_default(), line_length: config.line_length.unwrap_or(88), @@ -187,7 +213,6 @@ impl Settings { vec!["TODO".to_string(), "FIXME".to_string(), "XXX".to_string()] }), typing_modules: config.typing_modules.unwrap_or_default(), - update_check: config.update_check.unwrap_or_default(), // Plugins flake8_annotations: config .flake8_annotations @@ -227,17 +252,13 @@ impl Settings { Self { allowed_confusables: FxHashSet::from_iter([]), builtins: vec![], - cache_dir: cache_dir(path_dedot::CWD.as_path()), dummy_variable_rgx: Regex::new("^(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$").unwrap(), enabled: FxHashSet::from_iter([rule_code.clone()]), exclude: GlobSet::empty(), extend_exclude: GlobSet::empty(), external: FxHashSet::default(), - fix: false, - fix_only: false, fixable: FxHashSet::from_iter([rule_code]), force_exclude: false, - format: SerializationFormat::Text, ignore_init_module_imports: false, line_length: 88, namespace_packages: vec![], @@ -249,7 +270,6 @@ impl Settings { target_version: PythonVersion::Py310, task_tags: vec!["TODO".to_string(), "FIXME".to_string(), "XXX".to_string()], typing_modules: vec![], - update_check: false, flake8_annotations: flake8_annotations::settings::Settings::default(), flake8_bandit: flake8_bandit::settings::Settings::default(), flake8_bugbear: flake8_bugbear::settings::Settings::default(), @@ -273,17 +293,13 @@ impl Settings { Self { allowed_confusables: FxHashSet::from_iter([]), builtins: vec![], - cache_dir: cache_dir(path_dedot::CWD.as_path()), dummy_variable_rgx: Regex::new("^(_+|(_+[a-zA-Z0-9_]*[a-zA-Z0-9]+?))$").unwrap(), enabled: FxHashSet::from_iter(rule_codes.clone()), exclude: GlobSet::empty(), extend_exclude: GlobSet::empty(), external: FxHashSet::default(), - fix: false, - fix_only: false, fixable: FxHashSet::from_iter(rule_codes), force_exclude: false, - format: SerializationFormat::Text, ignore_init_module_imports: false, line_length: 88, namespace_packages: vec![], @@ -295,7 +311,6 @@ impl Settings { target_version: PythonVersion::Py310, task_tags: vec!["TODO".to_string(), "FIXME".to_string(), "XXX".to_string()], typing_modules: vec![], - update_check: false, flake8_annotations: flake8_annotations::settings::Settings::default(), flake8_bandit: flake8_bandit::settings::Settings::default(), flake8_bugbear: flake8_bugbear::settings::Settings::default(),