Skip to content

Commit

Permalink
Improve rule config resolution
Browse files Browse the repository at this point in the history 
Ruff allows rules to be enabled with `select` and disabled with
`ignore`, where the more specific rule selector takes precedence,
for example:

    `--select ALL --ignore E501` selects all rules except E501
    `--ignore ALL --select E501` selects only E501

(If both selectors have the same specificity ignore selectors
take precedence.)

Ruff always had two quirks:

* If `pyproject.toml` specified `ignore = ["E501"]` then you could
  previously not override that with `--select E501` on the command-line
  (since the resolution didn't take into account that the select was
  specified after the ignore).

* If `pyproject.toml` specified `select = ["E501"]` then you could
  previously not override that with `--ignore E` on the command-line
  (since the resolution didn't take into account that the ignore was
  specified after the select).

Since d067efe (astral-sh#1245)
`extend-select` and `extend-ignore` always override
`select` and `ignore` and are applied iteratively in pairs,
which introduced another quirk:

* If some `pyproject.toml` file specified `extend-select`
  or `extend-ignore`, `select` and `ignore` became pretty much
  unreliable after that with no way of resetting that.

This commit fixes all of these quirks by making later configuration
sources take precedence over earlier configuration sources.

While this is a breaking change, it fixes the broken status quo.
  • Loading branch information
@not-my-profile
not-my-profile committed Jan 29, 2023
1 parent 7b071f5 commit 8f048c5
Show file tree
Hide file tree
Showing 7 changed files with 173 additions and 191 deletions.
12 changes: 0 additions & 12 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2138,12 +2138,6 @@ extend-exclude = ["tests", "src/bad.py"]
A list of rule codes or prefixes to ignore, in addition to those
specified by `ignore`.

Note that `extend-ignore` is applied after resolving rules from
`ignore`/`select` and a less specific rule in `extend-ignore`
would overwrite a more specific rule in `select`. It is
recommended to only use `extend-ignore` when extending a
`pyproject.toml` file via `extend`.

**Default value**: `[]`

**Type**: `Vec<RuleSelector>`
Expand All @@ -2163,12 +2157,6 @@ extend-ignore = ["F841"]
A list of rule codes or prefixes to enable, in addition to those
specified by `select`.

Note that `extend-select` is applied after resolving rules from
`ignore`/`select` and a less specific rule in `extend-select`
would overwrite a more specific rule in `ignore`. It is
recommended to only use `extend-select` when extending a
`pyproject.toml` file via `extend`.

**Default value**: `[]`

**Type**: `Vec<RuleSelector>`
Expand Down
4 changes: 2 additions & 2 deletions ruff.schema.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
}
},
"extend-ignore": {
"description": "A list of rule codes or prefixes to ignore, in addition to those specified by `ignore`.\n\nNote that `extend-ignore` is applied after resolving rules from `ignore`/`select` and a less specific rule in `extend-ignore` would overwrite a more specific rule in `select`. It is recommended to only use `extend-ignore` when extending a `pyproject.toml` file via `extend`.",
"description": "A list of rule codes or prefixes to ignore, in addition to those specified by `ignore`.",
"type": [
"array",
"null"
Expand All @@ -77,7 +77,7 @@
}
},
"extend-select": {
"description": "A list of rule codes or prefixes to enable, in addition to those specified by `select`.\n\nNote that `extend-select` is applied after resolving rules from `ignore`/`select` and a less specific rule in `extend-select` would overwrite a more specific rule in `ignore`. It is recommended to only use `extend-select` when extending a `pyproject.toml` file via `extend`.",
"description": "A list of rule codes or prefixes to enable, in addition to those specified by `select`.",
"type": [
"array",
"null"
Expand Down
38 changes: 9 additions & 29 deletions ruff_cli/src/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use regex::Regex;
use ruff::logging::LogLevel;
use ruff::registry::Rule;
use ruff::resolver::ConfigProcessor;
use ruff::settings::configuration::RuleSelection;
use ruff::settings::types::{
FilePattern, PatternPrefixPair, PerFileIgnore, PythonVersion, SerializationFormat,
};
Expand Down Expand Up @@ -422,18 +423,20 @@ impl ConfigProcessor for &Overrides {
if let Some(fix_only) = &self.fix_only {
config.fix_only = Some(*fix_only);
}
if let Some(fixable) = &self.fixable {
config.fixable = Some(fixable.clone());
}
config.rule_selections.push(RuleSelection {
select: self.select.clone(),
ignore: self.ignore.clone(),
extend_select: self.extend_select.clone(),
extend_ignore: self.extend_ignore.clone(),
fixable: self.fixable.clone(),
unfixable: self.unfixable.clone(),
});
if let Some(format) = &self.format {
config.format = Some(*format);
}
if let Some(force_exclude) = &self.force_exclude {
config.force_exclude = Some(*force_exclude);
}
if let Some(ignore) = &self.ignore {
config.ignore = Some(ignore.clone());
}
if let Some(line_length) = &self.line_length {
config.line_length = Some(*line_length);
}
Expand All @@ -443,38 +446,15 @@ impl ConfigProcessor for &Overrides {
if let Some(respect_gitignore) = &self.respect_gitignore {
config.respect_gitignore = Some(*respect_gitignore);
}
if let Some(select) = &self.select {
config.select = Some(select.clone());
}
if let Some(show_source) = &self.show_source {
config.show_source = Some(*show_source);
}
if let Some(target_version) = &self.target_version {
config.target_version = Some(*target_version);
}
if let Some(unfixable) = &self.unfixable {
config.unfixable = Some(unfixable.clone());
}
if let Some(update_check) = &self.update_check {
config.update_check = Some(*update_check);
}
// Special-case: `extend_ignore` and `extend_select` are parallel arrays, so
// push an empty array if only one of the two is provided.
match (&self.extend_ignore, &self.extend_select) {
(Some(extend_ignore), Some(extend_select)) => {
config.extend_ignore.push(extend_ignore.clone());
config.extend_select.push(extend_select.clone());
}
(Some(extend_ignore), None) => {
config.extend_ignore.push(extend_ignore.clone());
config.extend_select.push(Vec::new());
}
(None, Some(extend_select)) => {
config.extend_ignore.push(Vec::new());
config.extend_select.push(extend_select.clone());
}
(None, None) => {}
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/rule_selector.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use schemars::JsonSchema;
use serde::de::{self, Visitor};
use serde::{Deserialize, Serialize};
use strum::IntoEnumIterator;
use strum_macros::EnumIter;

use crate::registry::{Rule, RuleCodePrefix, RuleIter};
use crate::rule_redirects::get_redirect;
Expand Down Expand Up @@ -171,7 +172,7 @@ impl RuleSelector {
}
}

#[derive(PartialEq, Eq, PartialOrd, Ord)]
#[derive(EnumIter, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) enum Specificity {
All,
Linter,
Expand Down
44 changes: 21 additions & 23 deletions src/settings/configuration.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,18 @@ use crate::settings::types::{
};

#[derive(Debug, Default)]
pub struct Configuration {
pub struct RuleSelection {
pub select: Option<Vec<RuleSelector>>,
pub ignore: Option<Vec<RuleSelector>>,
pub extend_select: Vec<Vec<RuleSelector>>,
pub extend_ignore: Vec<Vec<RuleSelector>>,
pub extend_select: Option<Vec<RuleSelector>>,
pub extend_ignore: Option<Vec<RuleSelector>>,
pub fixable: Option<Vec<RuleSelector>>,
pub unfixable: Option<Vec<RuleSelector>>,
}

#[derive(Debug, Default)]
pub struct Configuration {
pub rule_selections: Vec<RuleSelection>,
pub per_file_ignores: Option<Vec<PerFileIgnore>>,

pub allowed_confusables: Option<Vec<char>>,
Expand Down Expand Up @@ -88,6 +93,14 @@ impl Configuration {

pub fn from_options(options: Options, project_root: &Path) -> Result<Self> {
Ok(Configuration {
rule_selections: vec![RuleSelection {
select: options.select,
ignore: options.ignore,
extend_select: options.extend_select,
extend_ignore: options.extend_ignore,
fixable: options.fixable,
unfixable: options.unfixable,
}],
allowed_confusables: options.allowed_confusables,
builtins: options.builtins,
cache_dir: options
Expand Down Expand Up @@ -132,15 +145,11 @@ impl Configuration {
.collect()
})
.unwrap_or_default(),
extend_ignore: vec![options.extend_ignore.unwrap_or_default()],
extend_select: vec![options.extend_select.unwrap_or_default()],
external: options.external,
fix: options.fix,
fix_only: options.fix_only,
fixable: options.fixable,
format: options.format,
force_exclude: options.force_exclude,
ignore: options.ignore,
ignore_init_module_imports: options.ignore_init_module_imports,
line_length: options.line_length,
namespace_packages: options
Expand All @@ -157,7 +166,6 @@ impl Configuration {
}),
required_version: options.required_version,
respect_gitignore: options.respect_gitignore,
select: options.select,
show_source: options.show_source,
src: options
.src
Expand All @@ -166,7 +174,6 @@ impl Configuration {
target_version: options.target_version,
task_tags: options.task_tags,
typing_modules: options.typing_modules,
unfixable: options.unfixable,
update_check: options.update_check,
// Plugins
flake8_annotations: options.flake8_annotations,
Expand Down Expand Up @@ -194,6 +201,11 @@ impl Configuration {
#[must_use]
pub fn combine(self, config: Configuration) -> Self {
Self {
rule_selections: config
.rule_selections
.into_iter()
.chain(self.rule_selections.into_iter())
.collect(),
allowed_confusables: self.allowed_confusables.or(config.allowed_confusables),
builtins: self.builtins.or(config.builtins),
cache_dir: self.cache_dir.or(config.cache_dir),
Expand All @@ -205,23 +217,11 @@ impl Configuration {
.into_iter()
.chain(self.extend_exclude.into_iter())
.collect(),
extend_ignore: config
.extend_ignore
.into_iter()
.chain(self.extend_ignore.into_iter())
.collect(),
extend_select: config
.extend_select
.into_iter()
.chain(self.extend_select.into_iter())
.collect(),
external: self.external.or(config.external),
fix: self.fix.or(config.fix),
fix_only: self.fix_only.or(config.fix_only),
fixable: self.fixable.or(config.fixable),
format: self.format.or(config.format),
force_exclude: self.force_exclude.or(config.force_exclude),
ignore: self.ignore.or(config.ignore),
ignore_init_module_imports: self
.ignore_init_module_imports
.or(config.ignore_init_module_imports),
Expand All @@ -230,13 +230,11 @@ impl Configuration {
per_file_ignores: self.per_file_ignores.or(config.per_file_ignores),
required_version: self.required_version.or(config.required_version),
respect_gitignore: self.respect_gitignore.or(config.respect_gitignore),
select: self.select.or(config.select),
show_source: self.show_source.or(config.show_source),
src: self.src.or(config.src),
target_version: self.target_version.or(config.target_version),
task_tags: self.task_tags.or(config.task_tags),
typing_modules: self.typing_modules.or(config.typing_modules),
unfixable: self.unfixable.or(config.unfixable),
update_check: self.update_check.or(config.update_check),
// Plugins
flake8_annotations: self.flake8_annotations.or(config.flake8_annotations),
Expand Down
Loading

0 comments on commit 8f048c5

Please sign in to comment.