Skip to content

Commit

Permalink
Introduce LinterSettings
Browse files Browse the repository at this point in the history
## Stack Summary

This stack splits `Settings` into `FormatterSettings` and `LinterSettings` and moves it into `ruff_workspace`. This change is necessary to add the `FormatterSettings` to `Settings` without adding `ruff_python_formatter` as a dependency to `ruff_linter` (and the linter should not contain the formatter settings). 

A quick overview of our settings struct at play:

* `Options`: 1:1 representation of the options in the `pyproject.toml` or `ruff.toml`.  Used for deserialization.
* `Configuration`: Resolved `Options`, potentially merged from multiple configurations (when using `extend`). The representation is very close if not identical to the `Options`.
* `Settings`: The resolved configuration that uses a data format optimized for reading. Optional fields are initialized with their default values. Initialized by `Configuration::into_settings` .

The goal of this stack is to split `Settings` into tool-specific resolved `Settings` that are independent of each other. This comes at the advantage that the individual crates don't need to know anything about the other tools. The downside is that information gets duplicated between `Settings`. Right now the duplication is minimal (`line-length`, `tab-width`) but we may need to come up with a solution if more expensive data needs sharing.

This stack focuses on `Settings`. Splitting `Configuration` into some smaller structs is something I'll follow up on later. 

## PR Summary

This PR extracts the linter-specific settings into a new `LinterSettings` struct and adds it as a `linter` field to the `Settings` struct. This is in preparation for moving `Settings` from `ruff_linter` to `ruff_workspace`

## Test Plan

`cargo test`
  • Loading branch information
MichaReiser committed Sep 20, 2023
1 parent 222f1c3 commit b34278e
Show file tree
Hide file tree
Showing 101 changed files with 753 additions and 716 deletions.
10 changes: 5 additions & 5 deletions crates/ruff_benchmark/benches/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use ruff_benchmark::criterion::{
use ruff_benchmark::{TestCase, TestFile, TestFileDownloadError};
use ruff_linter::linter::lint_only;
use ruff_linter::settings::rule_table::RuleTable;
use ruff_linter::settings::{flags, Settings};
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::source_kind::SourceKind;
use ruff_linter::{registry::Rule, RuleSelector};
use ruff_python_ast::PySourceType;
Expand Down Expand Up @@ -41,7 +41,7 @@ fn create_test_cases() -> Result<Vec<TestCase>, TestFileDownloadError> {
])
}

fn benchmark_linter(mut group: BenchmarkGroup, settings: &Settings) {
fn benchmark_linter(mut group: BenchmarkGroup, settings: &LinterSettings) {
let test_cases = create_test_cases().unwrap();

for case in test_cases {
Expand Down Expand Up @@ -75,7 +75,7 @@ fn benchmark_linter(mut group: BenchmarkGroup, settings: &Settings) {

fn benchmark_default_rules(criterion: &mut Criterion) {
let group = criterion.benchmark_group("linter/default-rules");
benchmark_linter(group, &Settings::default());
benchmark_linter(group, &LinterSettings::default());
}

fn benchmark_all_rules(criterion: &mut Criterion) {
Expand All @@ -85,9 +85,9 @@ fn benchmark_all_rules(criterion: &mut Criterion) {
rules.disable(Rule::ShebangMissingExecutableFile);
rules.disable(Rule::ShebangNotExecutable);

let settings = Settings {
let settings = LinterSettings {
rules,
..Settings::default()
..LinterSettings::default()
};

let group = criterion.benchmark_group("linter/all-rules");
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_cli/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ mod tests {
let diagnostics = lint_path(
&path,
Some(&package_root),
&settings,
&settings.linter,
Some(&cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
Expand Down Expand Up @@ -450,7 +450,7 @@ mod tests {
got_diagnostics += lint_path(
&path,
Some(&package_root),
&settings,
&settings.linter,
Some(&cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
Expand Down Expand Up @@ -707,7 +707,7 @@ mod tests {
lint_path(
&self.package_root.join(path),
Some(&self.package_root),
&self.settings,
&self.settings.linter,
Some(cache),
flags::Noqa::Enabled,
flags::FixMode::Generate,
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_cli/src/commands/add_noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub(crate) fn add_noqa(
return None;
}
};
match add_noqa_to_path(path, package, &source_kind, source_type, settings) {
match add_noqa_to_path(path, package, &source_kind, source_type, &settings.linter) {
Ok(count) => Some(count),
Err(e) => {
error!("Failed to add noqa to {}: {e}", path.display());
Expand Down
15 changes: 9 additions & 6 deletions crates/ruff_cli/src/commands/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use rustc_hash::FxHashMap;
use ruff_diagnostics::Diagnostic;
use ruff_linter::message::Message;
use ruff_linter::registry::Rule;
use ruff_linter::settings::{flags, Settings};
use ruff_linter::settings::{flags, LinterSettings};
use ruff_linter::{fs, warn_user_once, IOError};
use ruff_python_ast::imports::ImportMap;
use ruff_source_file::SourceFileBuilder;
Expand Down Expand Up @@ -119,7 +119,7 @@ pub(crate) fn check(
}
});

lint_path(path, package, settings, cache, noqa, autofix).map_err(|e| {
lint_path(path, package, &settings.linter, cache, noqa, autofix).map_err(|e| {
(Some(path.to_owned()), {
let mut error = e.to_string();
for cause in e.chain() {
Expand All @@ -142,7 +142,7 @@ pub(crate) fn check(
.unwrap_or_else(|(path, message)| {
if let Some(path) = &path {
let settings = resolver.resolve(path, pyproject_config);
if settings.rules.enabled(Rule::IOError) {
if settings.linter.rules.enabled(Rule::IOError) {
let dummy =
SourceFileBuilder::new(path.to_string_lossy().as_ref(), "").finish();

Expand Down Expand Up @@ -195,7 +195,7 @@ pub(crate) fn check(
fn lint_path(
path: &Path,
package: Option<&Path>,
settings: &Settings,
settings: &LinterSettings,
cache: Option<&Cache>,
noqa: flags::Noqa,
autofix: flags::FixMode,
Expand Down Expand Up @@ -238,7 +238,7 @@ mod test {

use ruff_linter::message::{Emitter, EmitterContext, TextEmitter};
use ruff_linter::registry::Rule;
use ruff_linter::settings::{flags, Settings};
use ruff_linter::settings::{flags, LinterSettings, Settings};
use ruff_workspace::resolver::{PyprojectConfig, PyprojectDiscoveryStrategy};

use crate::args::CliOverrides;
Expand Down Expand Up @@ -268,7 +268,10 @@ mod test {
// Configure
let snapshot = format!("{}_{}", rule_code.noqa_code(), path);
// invalid pyproject.toml is not active by default
let settings = Settings::for_rules(vec![rule_code, Rule::InvalidPyprojectToml]);
let settings = Settings {
linter: LinterSettings::for_rules(vec![rule_code, Rule::InvalidPyprojectToml]),
..Settings::default()
};
let pyproject_config =
PyprojectConfig::new(PyprojectDiscoveryStrategy::Fixed, settings, None);

Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_cli/src/commands/check_stdin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub(crate) fn check_stdin(
}
}
let package_root = filename.and_then(Path::parent).and_then(|path| {
packaging::detect_package_root(path, &pyproject_config.settings.namespace_packages)
packaging::detect_package_root(path, &pyproject_config.settings.linter.namespace_packages)
});
let stdin = read_from_stdin()?;
let mut diagnostics = lint_stdin(
Expand Down
5 changes: 3 additions & 2 deletions crates/ruff_cli/src/commands/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,12 @@ pub(crate) fn format(

let resolved_settings = resolver.resolve(path, &pyproject_config);

let preview = match resolved_settings.preview {
// TODO(micha): Use `formatter` settings instead
let preview = match resolved_settings.linter.preview {
PreviewMode::Enabled => ruff_python_formatter::PreviewMode::Enabled,
PreviewMode::Disabled => ruff_python_formatter::PreviewMode::Disabled,
};
let line_length = resolved_settings.line_length;
let line_length = resolved_settings.linter.line_length;

let options = PyFormatOptions::from_source_type(source_type)
.with_line_width(LineWidth::from(NonZeroU16::from(line_length)))
Expand Down
5 changes: 3 additions & 2 deletions crates/ruff_cli/src/commands/format_stdin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,12 @@ pub(crate) fn format_stdin(cli: &FormatArguments, overrides: &CliOverrides) -> R
// Format the file.
let path = cli.stdin_filename.as_deref();

let preview = match pyproject_config.settings.preview {
// TODO(micha): Use Formatter settings
let preview = match pyproject_config.settings.linter.preview {
PreviewMode::Enabled => ruff_python_formatter::PreviewMode::Enabled,
PreviewMode::Disabled => ruff_python_formatter::PreviewMode::Disabled,
};
let line_length = pyproject_config.settings.line_length;
let line_length = pyproject_config.settings.linter.line_length;

let options = path
.map(PyFormatOptions::from_extension)
Expand Down
14 changes: 7 additions & 7 deletions crates/ruff_cli/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use ruff_linter::logging::DisplayParseError;
use ruff_linter::message::Message;
use ruff_linter::pyproject_toml::lint_pyproject_toml;
use ruff_linter::registry::AsRule;
use ruff_linter::settings::{flags, Settings};
use ruff_linter::settings::{flags, LinterSettings, Settings};
use ruff_linter::source_kind::SourceKind;
use ruff_linter::{fs, IOError, SyntaxError};
use ruff_macros::CacheKey;
Expand Down Expand Up @@ -85,7 +85,7 @@ impl Diagnostics {
pub(crate) fn from_source_error(
err: &SourceExtractionError,
path: Option<&Path>,
settings: &Settings,
settings: &LinterSettings,
) -> Self {
let diagnostic = Diagnostic::from(err);
if settings.rules.enabled(diagnostic.kind.rule()) {
Expand Down Expand Up @@ -143,7 +143,7 @@ impl AddAssign for Diagnostics {
pub(crate) fn lint_path(
path: &Path,
package: Option<&Path>,
settings: &Settings,
settings: &LinterSettings,
cache: Option<&Cache>,
noqa: flags::Noqa,
autofix: flags::FixMode,
Expand Down Expand Up @@ -381,7 +381,7 @@ pub(crate) fn lint_stdin(
Ok(Some(sources)) => sources,
Ok(None) => return Ok(Diagnostics::default()),
Err(err) => {
return Ok(Diagnostics::from_source_error(&err, path, settings));
return Ok(Diagnostics::from_source_error(&err, path, &settings.linter));
}
};

Expand All @@ -401,7 +401,7 @@ pub(crate) fn lint_stdin(
path.unwrap_or_else(|| Path::new("-")),
package,
noqa,
settings,
&settings.linter,
&source_kind,
source_type,
) {
Expand Down Expand Up @@ -438,7 +438,7 @@ pub(crate) fn lint_stdin(
let result = lint_only(
path.unwrap_or_else(|| Path::new("-")),
package,
settings,
&settings.linter,
noqa,
&source_kind,
source_type,
Expand All @@ -456,7 +456,7 @@ pub(crate) fn lint_stdin(
let result = lint_only(
path.unwrap_or_else(|| Path::new("-")),
package,
settings,
&settings.linter,
noqa,
&source_kind,
source_type,
Expand Down
7 changes: 5 additions & 2 deletions crates/ruff_dev/src/format_dev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,11 @@ fn format_dir_entry(

let settings = resolver.resolve(&path, pyproject_config);
// That's a bad way of doing this but it's not worth doing something better for format_dev
if settings.line_length != LineLength::default() {
options = options.with_line_width(LineWidth::from(NonZeroU16::from(settings.line_length)));
// TODO(micha) use formatter settings instead
if settings.linter.line_length != LineLength::default() {
options = options.with_line_width(LineWidth::from(NonZeroU16::from(
settings.linter.line_length,
)));
}

// Handle panics (mostly in `debug_assert!`)
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::InvalidIndexType) {
ruff::rules::invalid_index_type(checker, subscript);
}
if checker.settings.rules.enabled(Rule::SliceCopy) {
if checker.enabled(Rule::SliceCopy) {
refurb::rules::slice_copy(checker, subscript);
}

Expand Down
10 changes: 5 additions & 5 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ use crate::importer::Importer;
use crate::noqa::NoqaMapping;
use crate::registry::Rule;
use crate::rules::{flake8_pyi, flake8_type_checking, pyflakes, pyupgrade};
use crate::settings::{flags, Settings};
use crate::settings::{flags, LinterSettings};
use crate::{docstrings, noqa};

mod analyze;
Expand All @@ -85,8 +85,8 @@ pub(crate) struct Checker<'a> {
/// The [`NoqaMapping`] for the current analysis (i.e., the mapping from line number to
/// suppression commented line number).
noqa_line_for: &'a NoqaMapping,
/// The [`Settings`] for the current analysis, including the enabled rules.
pub(crate) settings: &'a Settings,
/// The [`LinterSettings`] for the current analysis, including the enabled rules.
pub(crate) settings: &'a LinterSettings,
/// The [`Locator`] for the current file, which enables extraction of source code from byte
/// offsets.
locator: &'a Locator<'a>,
Expand All @@ -110,7 +110,7 @@ pub(crate) struct Checker<'a> {
impl<'a> Checker<'a> {
#[allow(clippy::too_many_arguments)]
pub(crate) fn new(
settings: &'a Settings,
settings: &'a LinterSettings,
noqa_line_for: &'a NoqaMapping,
noqa: flags::Noqa,
path: &'a Path,
Expand Down Expand Up @@ -1929,7 +1929,7 @@ pub(crate) fn check_ast(
stylist: &Stylist,
indexer: &Indexer,
noqa_line_for: &NoqaMapping,
settings: &Settings,
settings: &LinterSettings,
noqa: flags::Noqa,
path: &Path,
package: Option<&Path>,
Expand Down
13 changes: 5 additions & 8 deletions crates/ruff_linter/src/checkers/filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,20 @@ use ruff_diagnostics::Diagnostic;
use crate::registry::Rule;
use crate::rules::flake8_no_pep420::rules::implicit_namespace_package;
use crate::rules::pep8_naming::rules::invalid_module_name;
use crate::settings::Settings;
use crate::settings::LinterSettings;

pub(crate) fn check_file_path(
path: &Path,
package: Option<&Path>,
settings: &Settings,
settings: &LinterSettings,
) -> Vec<Diagnostic> {
let mut diagnostics: Vec<Diagnostic> = vec![];

// flake8-no-pep420
if settings.rules.enabled(Rule::ImplicitNamespacePackage) {
if let Some(diagnostic) = implicit_namespace_package(
path,
package,
&settings.file_resolver.project_root,
&settings.src,
) {
if let Some(diagnostic) =
implicit_namespace_package(path, package, &settings.project_root, &settings.src)
{
diagnostics.push(diagnostic);
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::directives::IsortDirectives;
use crate::registry::Rule;
use crate::rules::isort;
use crate::rules::isort::block::{Block, BlockBuilder};
use crate::settings::Settings;
use crate::settings::LinterSettings;
use crate::source_kind::SourceKind;

fn extract_import_map(path: &Path, package: Option<&Path>, blocks: &[&Block]) -> Option<ImportMap> {
Expand Down Expand Up @@ -81,7 +81,7 @@ pub(crate) fn check_imports(
locator: &Locator,
indexer: &Indexer,
directives: &IsortDirectives,
settings: &Settings,
settings: &LinterSettings,
stylist: &Stylist,
path: &Path,
package: Option<&Path>,
Expand Down
8 changes: 4 additions & 4 deletions crates/ruff_linter/src/checkers/logical_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::rules::pycodestyle::rules::logical_lines::{
whitespace_around_keywords, whitespace_around_named_parameter_equals,
whitespace_before_comment, whitespace_before_parameters, LogicalLines, TokenFlags,
};
use crate::settings::Settings;
use crate::settings::LinterSettings;

/// Return the amount of indentation, expanding tabs to the next multiple of 8.
fn expand_indent(line: &str) -> usize {
Expand All @@ -34,7 +34,7 @@ pub(crate) fn check_logical_lines(
tokens: &[LexResult],
locator: &Locator,
stylist: &Stylist,
settings: &Settings,
settings: &LinterSettings,
) -> Vec<Diagnostic> {
let mut context = LogicalLinesContext::new(settings);

Expand Down Expand Up @@ -132,12 +132,12 @@ pub(crate) fn check_logical_lines(

#[derive(Debug, Clone)]
pub(crate) struct LogicalLinesContext<'a> {
settings: &'a Settings,
settings: &'a LinterSettings,
diagnostics: Vec<Diagnostic>,
}

impl<'a> LogicalLinesContext<'a> {
fn new(settings: &'a Settings) -> Self {
fn new(settings: &'a LinterSettings) -> Self {
Self {
settings,
diagnostics: Vec::new(),
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use crate::noqa::{Directive, FileExemption, NoqaDirectives, NoqaMapping};
use crate::registry::{AsRule, Rule};
use crate::rule_redirects::get_redirect_target;
use crate::rules::ruff::rules::{UnusedCodes, UnusedNOQA};
use crate::settings::Settings;
use crate::settings::LinterSettings;

pub(crate) fn check_noqa(
diagnostics: &mut Vec<Diagnostic>,
Expand All @@ -23,7 +23,7 @@ pub(crate) fn check_noqa(
comment_ranges: &CommentRanges,
noqa_line_for: &NoqaMapping,
analyze_directives: bool,
settings: &Settings,
settings: &LinterSettings,
) -> Vec<usize> {
// Identify any codes that are globally exempted (within the current file).
let exemption = FileExemption::try_extract(locator.contents(), comment_ranges, path, locator);
Expand Down

0 comments on commit b34278e

Please sign in to comment.