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

[ruff] Implement incorrectly-parenthesized-tuple-in-subscript (RUF031) #12480

Merged
merged 35 commits into from
Aug 7, 2024
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8acd409
add test fixture for lint rule
dylwil3 Jul 23, 2024
e9aa21c
add test cases with newlines
dylwil3 Jul 23, 2024
d59408f
add module to check for parens around tuples in getitem calls
dylwil3 Jul 23, 2024
7882fcd
add to codes as RUF031
dylwil3 Jul 23, 2024
be86fe9
add pointer to test case for rule
dylwil3 Jul 23, 2024
55c334a
implement rule for subscript expressions in checker
dylwil3 Jul 23, 2024
ec086e8
update schema
dylwil3 Jul 23, 2024
284a194
add snapshot for RUF031
dylwil3 Jul 23, 2024
9513c9c
reformat example
dylwil3 Jul 23, 2024
5b0a92a
begin adding settings functionality to ruff ruleset
dylwil3 Aug 2, 2024
61106a6
rename rule and add config for ruff
dylwil3 Aug 2, 2024
f3db34e
update rule fix and violation logic to respect configuration
dylwil3 Aug 2, 2024
3f5d5bb
update snapshot
dylwil3 Aug 2, 2024
a0e7b04
update schema
dylwil3 Aug 2, 2024
0c339ab
fix ruff settings namespace copypasta mistake and regenerate snapshot
dylwil3 Aug 3, 2024
c2e350b
add test case with parens on setting
dylwil3 Aug 3, 2024
5e1e0a4
make ruff an option group
dylwil3 Aug 3, 2024
a696961
rename rule to satisfy convention
dylwil3 Aug 3, 2024
209add0
change rule and config option names
dylwil3 Aug 5, 2024
bf5947d
typo
dylwil3 Aug 7, 2024
2a895b6
Use `let .. else`
dylwil3 Aug 7, 2024
dedda82
resolve comments
dylwil3 Aug 7, 2024
d8ea3a2
update snapshot
dylwil3 Aug 7, 2024
f7ff6a3
treat single element tuples as well
dylwil3 Aug 7, 2024
dd3d881
update snapshot
dylwil3 Aug 7, 2024
cec2241
clippy
dylwil3 Aug 7, 2024
96fcb28
typo and subscript instead of getitem
dylwil3 Aug 7, 2024
bbe1801
update snapshot again
dylwil3 Aug 7, 2024
5a83811
change name of setting
dylwil3 Aug 7, 2024
4cc9eb5
update schema
dylwil3 Aug 7, 2024
767ae99
update docstrings
dylwil3 Aug 7, 2024
b95f068
rename module and remove clippy allow
dylwil3 Aug 7, 2024
84afc6e
match to if/else
dylwil3 Aug 7, 2024
ccf47a7
Apply suggestions from code review
AlexWaygood Aug 7, 2024
4ba055d
final nits
AlexWaygood Aug 7, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ linter.pylint.max_statements = 50
linter.pylint.max_public_methods = 20
linter.pylint.max_locals = 15
linter.pyupgrade.keep_runtime_typing = false
linter.ruff.parenthesize_tuple_in_subscript = false

# Formatter Settings
formatter.exclude = []
Expand Down
28 changes: 28 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF031.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
d = {(1,2):"a",(3,4):"b",(5,6,7):"c",(8,):"d"}
d[(1,2)]
d[(
1,
2
)]
dylwil3 marked this conversation as resolved.
Show resolved Hide resolved
d[
1,
2
]
d[(2,4)]
d[(5,6,7)]
d[(8,)]
d[tuple(1,2)]
d[tuple(8)]
d[1,2]
d[3,4]
d[5,6,7]
e = {((1,2),(3,4)):"a"}
e[((1,2),(3,4))]
e[(1,2),(3,4)]

token_features[
(window_position, feature_name)
] = self._extract_raw_features_from_token

d[1,]
d[(1,)]
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
d = {(1,2):"a",(3,4):"b",(5,6,7):"c",(8,):"d"}
d[(1,2)]
d[(
1,
2
)]
d[
1,
2
]
d[(2,4)]
d[(5,6,7)]
d[(8,)]
d[tuple(1,2)]
d[tuple(8)]
d[1,2]
d[3,4]
d[5,6,7]
e = {((1,2),(3,4)):"a"}
e[((1,2),(3,4))]
e[(1,2),(3,4)]

token_features[
(window_position, feature_name)
] = self._extract_raw_features_from_token
d[1,]
d[(1,)]
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
refurb::rules::fstring_number_format(checker, subscript);
}

if checker.enabled(Rule::IncorrectlyParenthesizedTupleInSubscript) {
ruff::rules::subscript_with_parenthesized_tuple(checker, subscript);
}

pandas_vet::rules::subscript(checker, value, expr);
}
Expr::Tuple(ast::ExprTuple {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::InvalidFormatterSuppressionComment),
(Ruff, "029") => (RuleGroup::Preview, rules::ruff::rules::UnusedAsync),
(Ruff, "030") => (RuleGroup::Preview, rules::ruff::rules::AssertWithPrintMessage),
(Ruff, "031") => (RuleGroup::Preview, rules::ruff::rules::IncorrectlyParenthesizedTupleInSubscript),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "101") => (RuleGroup::Preview, rules::ruff::rules::RedirectedNOQA),

Expand Down
18 changes: 18 additions & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! Ruff-specific rules.

pub(crate) mod rules;
pub mod settings;
pub(crate) mod typing;

#[cfg(test)]
Expand All @@ -19,6 +20,7 @@ mod tests {
use crate::settings::types::{
CompiledPerFileIgnoreList, PerFileIgnore, PreviewMode, PythonVersion,
};
use crate::settings::LinterSettings;
use crate::test::{test_path, test_resource_path};
use crate::{assert_messages, settings};

Expand Down Expand Up @@ -55,6 +57,7 @@ mod tests {
#[test_case(Rule::InvalidFormatterSuppressionComment, Path::new("RUF028.py"))]
#[test_case(Rule::UnusedAsync, Path::new("RUF029.py"))]
#[test_case(Rule::AssertWithPrintMessage, Path::new("RUF030.py"))]
#[test_case(Rule::IncorrectlyParenthesizedTupleInSubscript, Path::new("RUF031.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand All @@ -66,6 +69,21 @@ mod tests {
Ok(())
}

#[test]
fn prefer_parentheses_getitem_tuple() -> Result<()> {
let diagnostics = test_path(
Path::new("ruff/RUF031_prefer_parens.py"),
&LinterSettings {
ruff: super::settings::Settings {
parenthesize_tuple_in_subscript: true,
},
..LinterSettings::for_rule(Rule::IncorrectlyParenthesizedTupleInSubscript)
},
)?;
assert_messages!(diagnostics);
Ok(())
}

#[test_case(Path::new("RUF013_0.py"))]
#[test_case(Path::new("RUF013_1.py"))]
fn implicit_optional_py39(path: &Path) -> Result<()> {
Expand Down
dylwil3 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// In case we want to change the boolean setting for
// this rule to an enum, this will make the code change
// just a little simpler.
#![allow(clippy::match_bool)]
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to use an if...else over a match instead of suppressing the clippy warning.

Copy link
Member

@AlexWaygood AlexWaygood Aug 7, 2024

Choose a reason for hiding this comment

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

Agreed. Sometimes in this kind of situation it can be more readable to introduce a custom enum and match on that instead, e.g.

enum ParenthesesPreference {
    Keep,
    Remove,
}

but here I'm not sure I really see the need; I think I'd just stick with bool

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this has been marked as resolved, but the comment hasn't been addressed


use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::ExprSubscript;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for use or omission of parentheses around tuples in subscripts,
/// depending on the setting [`lint.ruff.parenthesize-tuple-in-subscript`]. By default, the use of parentheses
/// is considered a violation.
///
/// ## Why is this bad?
/// It is good to be consistent and, depending on the codebase, one or the other
/// convention may be preferred.
///
/// ## Example
///
/// ```python
/// directions = {(0, 1): "North", (-1, 0): "East", (0, -1): "South", (1, 0): "West"}
/// directions[(0, 1)]
/// ```
///
/// Use instead (with default setting):
///
/// ```python
/// directions = {(0, 1): "North", (-1, 0): "East", (0, -1): "South", (1, 0): "West"}
/// directions[0, 1]
/// ```

#[violation]
pub struct IncorrectlyParenthesizedTupleInSubscript {
prefer_parentheses: bool,
}

impl AlwaysFixableViolation for IncorrectlyParenthesizedTupleInSubscript {
#[derive_message_formats]
fn message(&self) -> String {
match self.prefer_parentheses {
true => format!("Use parentheses for tuples in subscripts."),
false => format!("Avoid parentheses for tuples in scubscripts."),
}
}

fn fix_title(&self) -> String {
match self.prefer_parentheses {
true => "Add parentheses around tuple in subscript.".to_string(),
false => "Remove parentheses from tuple in subscript.".to_string(),
}
}
}

/// RUF031
pub(crate) fn subscript_with_parenthesized_tuple(checker: &mut Checker, subscript: &ExprSubscript) {
let prefer_parentheses = checker.settings.ruff.parenthesize_tuple_in_subscript;
let Some(tuple_index) = subscript.slice.as_tuple_expr() else {
return;
};
if tuple_index.parenthesized != prefer_parentheses {
let locator = checker.locator();
let source_range = subscript.slice.range();
let new_source = match prefer_parentheses {
true => {
format!("({})", locator.slice(source_range))
}
false => locator.slice(source_range)[1..source_range.len().to_usize() - 1].to_string(),
};
let edit = Edit::range_replacement(new_source, source_range);
checker.diagnostics.push(
Diagnostic::new(
IncorrectlyParenthesizedTupleInSubscript { prefer_parentheses },
source_range,
)
.with_fix(Fix::safe_edit(edit)),
);
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ pub(crate) use default_factory_kwarg::*;
pub(crate) use explicit_f_string_type_conversion::*;
pub(crate) use function_call_in_dataclass_default::*;
pub(crate) use implicit_optional::*;
pub(crate) use incorrectly_parenthesized_tuple_in_getitem::*;
pub(crate) use invalid_formatter_suppression_comment::*;
pub(crate) use invalid_index_type::*;
pub(crate) use invalid_pyproject_toml::*;
Expand Down Expand Up @@ -41,6 +42,7 @@ mod explicit_f_string_type_conversion;
mod function_call_in_dataclass_default;
mod helpers;
mod implicit_optional;
mod incorrectly_parenthesized_tuple_in_getitem;
mod invalid_formatter_suppression_comment;
mod invalid_index_type;
mod invalid_pyproject_toml;
Expand Down
23 changes: 23 additions & 0 deletions crates/ruff_linter/src/rules/ruff/settings.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//! Settings for the `ruff` plugin.

use crate::display_settings;
use ruff_macros::CacheKey;
use std::fmt;

#[derive(Debug, Clone, CacheKey, Default)]
pub struct Settings {
pub parenthesize_tuple_in_subscript: bool,
}

impl fmt::Display for Settings {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
display_settings! {
formatter = f,
namespace = "linter.ruff",
fields = [
self.parenthesize_tuple_in_subscript
]
}
Ok(())
}
}
Loading
Loading