Skip to content
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

Allow overriding pydocstyle convention rules #8586

Merged
merged 8 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions crates/ruff_cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1564,3 +1564,62 @@ extend-safe-fixes = ["UP03"]

Ok(())
}

#[test]
fn check_docstring_conventions_overrides() -> Result<()> {
// But if we explicitly select it, we override the convention
let tempdir = TempDir::new()?;
let ruff_toml = tempdir.path().join("ruff.toml");
fs::write(
&ruff_toml,
r#"
[lint.pydocstyle]
convention = "numpy"
"#,
)?;

let stdin = r#"
def log(x, base) -> float:
"""Calculate natural log of a value

Parameters
----------
x :
Hello
"""
return math.log(x)
"#;

// If we only select the prefix, then everything passes
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I found some of this to be a bit repetitive -- would you be open to a PR that tries to refactor some of this Command construction into a custom builder struct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm open to the idea!

.args(["check", "-", "--config"])
.arg(&ruff_toml)
.args(["--output-format", "text", "--no-cache", "--select", "D41"])
.pass_stdin(stdin),
@r###"
success: true
exit_code: 0
----- stdout -----

----- stderr -----
"###
);

// But if we select the exact code, we get an error
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(["check", "-", "--config"])
.arg(&ruff_toml)
.args(["--output-format", "text", "--no-cache", "--select", "D417"])
.pass_stdin(stdin),
@r###"
success: false
exit_code: 1
----- stdout -----
-:2:5: D417 Missing argument description in the docstring for `log`: `base`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, D417 was the rule I was surprised was disabled in the numpy convention. It looks like there's specific code written to handle some of the quirks (e.g.

// Notably, NumPy lets you put multiple parameters of the same type on the same
), so it seemed odd that it was disabled.

Found 1 error.

----- stderr -----
"###
);
Ok(())
}
116 changes: 113 additions & 3 deletions crates/ruff_workspace/src/configuration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,10 @@ impl LintConfiguration {
let mut deprecated_nursery_selectors = FxHashSet::default();
let mut ignored_preview_selectors = FxHashSet::default();

// Track which docstring rules are specifically enabled
// which lets us override the docstring convention ignore-list
let mut docstring_overrides: FxHashSet<Rule> = FxHashSet::default();

for selection in &self.rule_selections {
// If a selection only specifies extend-select we cannot directly
// apply its rule selectors to the select_set because we firstly have
Expand All @@ -716,6 +720,8 @@ impl LintConfiguration {
let mut select_map_updates: FxHashMap<Rule, bool> = FxHashMap::default();
let mut fixable_map_updates: FxHashMap<Rule, bool> = FxHashMap::default();

let mut docstring_override_updates: FxHashSet<Rule> = FxHashSet::default();

let carriedover_ignores = carryover_ignores.take();
let carriedover_unfixables = carryover_unfixables.take();

Expand All @@ -730,6 +736,10 @@ impl LintConfiguration {
{
for rule in selector.rules(&preview) {
select_map_updates.insert(rule, true);

if spec == Specificity::Rule {
docstring_override_updates.insert(rule);
}
}
}
for selector in selection
Expand All @@ -742,6 +752,7 @@ impl LintConfiguration {
select_map_updates.insert(rule, false);
}
}

// Apply the same logic to `fixable` and `unfixable`.
for selector in selection
.fixable
Expand Down Expand Up @@ -780,6 +791,8 @@ impl LintConfiguration {
{
carryover_ignores = Some(&selection.ignore);
}

docstring_overrides = docstring_override_updates;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be fine to omit docstring_override_updates and just add to docstring_overrides whenever we see a direct selector. In other words, I don't know if this "reset" behavior is important for the purpose of these overrides. I'm trying to think of a case where this would matter.

As an example of why this exists (to the best of my memory): imagine you have select=D and ignore=D401 in your configuration file. If you then pass --select D4, we want to ignore the ignore. So we treat a --select as resetting the selection chain.

Eh, thinking about it further, what you have here is probably right. Imagine you have select=D415 in your configuration file, and then you run with --select=D. We probably still want to enforce the configuration override for D415 in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I was actually thinking about this in terms of having multiple ruff.tomls (e.g. in a monorepo or something). In that case, I think the intended behavior is that select "clears" the state of the "parent" ruff.tomls, and personally I think it'd be confusing if the docstring conventions were the only counter-example to that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

} else {
// Otherwise we apply the updates on top of the existing select_set.
for (rule, enabled) in select_map_updates {
Expand All @@ -789,6 +802,10 @@ impl LintConfiguration {
select_set.remove(rule);
}
}

for rule in docstring_override_updates {
docstring_overrides.insert(rule);
}
}

// Apply the same logic to `fixable` and `unfixable`.
Expand Down Expand Up @@ -881,15 +898,17 @@ impl LintConfiguration {
rules.enable(rule, fix);
}

// If a docstring convention is specified, force-disable any incompatible error
// codes.
// If a docstring convention is specified, disable any incompatible error
// codes unless we are specifically overridden.
if let Some(convention) = self
.pydocstyle
.as_ref()
.and_then(|pydocstyle| pydocstyle.convention)
{
for rule in convention.rules_to_be_ignored() {
rules.disable(*rule);
if !docstring_overrides.contains(rule) {
rules.disable(*rule);
}
}
}

Expand Down Expand Up @@ -1073,11 +1092,13 @@ pub fn resolve_src(src: &[String], project_root: &Path) -> Result<Vec<PathBuf>>
#[cfg(test)]
mod tests {
use crate::configuration::{LintConfiguration, RuleSelection};
use crate::options::PydocstyleOptions;
use ruff_linter::codes::{Flake8Copyright, Pycodestyle, Refurb};
use ruff_linter::registry::{Linter, Rule, RuleSet};
use ruff_linter::rule_selector::PreviewOptions;
use ruff_linter::settings::types::PreviewMode;
use ruff_linter::RuleSelector;
use std::str::FromStr;

const NURSERY_RULES: &[Rule] = &[
Rule::MissingCopyrightNotice,
Expand Down Expand Up @@ -1569,4 +1590,93 @@ mod tests {
let expected = RuleSet::from_rules(NURSERY_RULES);
assert_eq!(actual, expected);
}

#[test]
fn select_docstring_convention_override() {
fn _check_docstring_override(
rule_selections: Vec<RuleSelection>,
should_be_overridden: bool,
) {
use ruff_linter::rules::pydocstyle::settings::Convention;

let config = LintConfiguration {
rule_selections,
pydocstyle: Some(PydocstyleOptions {
convention: Some(Convention::Numpy),
..PydocstyleOptions::default()
}),
..LintConfiguration::default()
};

let mut expected = RuleSet::from_rules(&[
Rule::from_code("D410").unwrap(),
Rule::from_code("D411").unwrap(),
Rule::from_code("D412").unwrap(),
Rule::from_code("D414").unwrap(),
Rule::from_code("D418").unwrap(),
Rule::from_code("D419").unwrap(),
]);
if should_be_overridden {
expected.insert(Rule::from_code("D417").unwrap());
}
assert_eq!(
config
.as_rule_table(PreviewMode::Disabled)
.iter_enabled()
.collect::<RuleSet>(),
expected,
);
}

let d41 = RuleSelector::from_str("D41").unwrap();
let d417 = RuleSelector::from_str("D417").unwrap();

// D417 does not appear with prefix
_check_docstring_override(
vec![RuleSelection {
select: Some(vec![d41.clone()]),
..RuleSelection::default()
}],
false,
);

// But does appear with rule selector
_check_docstring_override(
vec![RuleSelection {
select: Some(vec![d41.clone(), d417.clone()]),
..RuleSelection::default()
}],
true,
);

// But disappears if there's another select
_check_docstring_override(
vec![
RuleSelection {
select: Some(vec![d417.clone()]),
..RuleSelection::default()
},
RuleSelection {
select: Some(vec![d41.clone()]),
..RuleSelection::default()
},
],
false,
);

// But an extend-select is ok
_check_docstring_override(
vec![
RuleSelection {
select: Some(vec![d417.clone()]),
..RuleSelection::default()
},
RuleSelection {
extend_select: vec![d41.clone()],
..RuleSelection::default()
},
],
true,
);
}
}
23 changes: 20 additions & 3 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2405,9 +2405,26 @@ pub struct PydocstyleOptions {
/// convention = "google"
/// ```
///
/// As conventions force-disable all rules not included in the convention,
/// enabling _additional_ rules on top of a convention is currently
/// unsupported.
/// If you must, you can enable rules not included in a convention by
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there another place I should update the docs, or is this the right place?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right place :)

/// requesting the specific error code:
///
/// ```toml
/// [tool.ruff.lint]
/// extend-select = ["D400"] # will enable D400 rule
///
/// [tool.ruff.lint.pydocstyle]
/// convention = "google"
/// ```
///
/// Just adding a rule prefix will not enable convention-specified rules.
///
/// ```toml
/// [tool.ruff.lint]
/// extend-select = ["D40"] # will *not* enable D400 rule
///
/// [tool.ruff.lint.pydocstyle]
/// convention = "google"
/// ```
#[option(
default = r#"null"#,
value_type = r#""google" | "numpy" | "pep257""#,
Expand Down
2 changes: 1 addition & 1 deletion ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading