From bd4adcd501b8c5b84741a8244a4a76b0f80cbe73 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Sep 2024 22:41:53 -0500 Subject: [PATCH 1/7] update test fixture --- .../ruff_linter/resources/test/fixtures/refurb/FURB188.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py index 3437d5c56bec4f..ddb1555c85d05b 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py @@ -151,4 +151,10 @@ def remove_prefix_comparable_literal_expr() -> None: def shadow_builtins(filename: str, extension: str) -> None: from builtins import len as builtins_len - return filename[:-builtins_len(extension)] if filename.endswith(extension) else filename \ No newline at end of file + return filename[:-builtins_len(extension)] if filename.endswith(extension) else filename + +def ignore_step(): + text = "!x!y!z" + if text.startswith("!"): + text = text[1::2] + print(text) \ No newline at end of file From 0683a7e3ef171c52ec63854ee2c83169cc7a42aa Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Sep 2024 23:04:43 -0500 Subject: [PATCH 2/7] update fixture --- .../resources/test/fixtures/refurb/FURB188.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py index ddb1555c85d05b..45a39257f3255b 100644 --- a/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py +++ b/crates/ruff_linter/resources/test/fixtures/refurb/FURB188.py @@ -153,6 +153,18 @@ def shadow_builtins(filename: str, extension: str) -> None: return filename[:-builtins_len(extension)] if filename.endswith(extension) else filename +def okay_steps(): + text = "!x!y!z" + if text.startswith("!"): + text = text[1::1] + if text.startswith("!"): + text = text[1::True] + if text.startswith("!"): + text = text[1::None] + print(text) + + +# this should be skipped def ignore_step(): text = "!x!y!z" if text.startswith("!"): From e32f699e283aca6c198bb8bf67439e39b0b429b2 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Sep 2024 23:06:52 -0500 Subject: [PATCH 3/7] early exit for nontrivial step in slice --- .../rules/slice_to_remove_prefix_or_suffix.rs | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs index e3fc11bfb6a368..ee0395fdec106b 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs @@ -248,6 +248,30 @@ fn affix_removal_data<'a>( return None; } let slice = slice.as_slice_expr()?; + + // Exit early _unless_ slice step is... + if slice + .step + .as_ref() + // omitted + .is_some_and(|step| match step.as_ref() { + // equal to 1 + ast::Expr::NumberLiteral(ast::ExprNumberLiteral { + range: _, + value: ast::Number::Int(x), + }) => !(x.as_u8().is_some_and(|val| val == 1)), + // or `None` or `True` + ast::Expr::NoneLiteral(_) + | ast::Expr::BooleanLiteral(ast::ExprBooleanLiteral { + range: _, + value: true, + }) => false, + _ => true, + }) + { + return None; + }; + let compr_test_expr = ast::comparable::ComparableExpr::from( &test.as_call_expr()?.func.as_attribute_expr()?.value, ); From 31cac983df233b5d19e7608661799b9e54e819f0 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Sep 2024 23:06:58 -0500 Subject: [PATCH 4/7] update snapshot --- ...es__refurb__tests__FURB188_FURB188.py.snap | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap index 3103e5f2723adb..89a0c17633e70a 100644 --- a/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap +++ b/crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB188_FURB188.py.snap @@ -166,6 +166,8 @@ FURB188.py:154:12: FURB188 [*] Prefer `removesuffix` over conditionally replacin 153 | 154 | return filename[:-builtins_len(extension)] if filename.endswith(extension) else filename | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB188 +155 | +156 | def okay_steps(): | = help: Use removesuffix instead of ternary expression conditional upon endswith. @@ -175,3 +177,77 @@ FURB188.py:154:12: FURB188 [*] Prefer `removesuffix` over conditionally replacin 153 153 | 154 |- return filename[:-builtins_len(extension)] if filename.endswith(extension) else filename 154 |+ return filename.removesuffix(extension) +155 155 | +156 156 | def okay_steps(): +157 157 | text = "!x!y!z" + +FURB188.py:158:5: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice. + | +156 | def okay_steps(): +157 | text = "!x!y!z" +158 | if text.startswith("!"): + | _____^ +159 | | text = text[1::1] + | |_________________________^ FURB188 +160 | if text.startswith("!"): +161 | text = text[1::True] + | + = help: Use removeprefix instead of assignment conditional upon startswith. + +ℹ Safe fix +155 155 | +156 156 | def okay_steps(): +157 157 | text = "!x!y!z" +158 |- if text.startswith("!"): +159 |- text = text[1::1] + 158 |+ text = text.removeprefix("!") +160 159 | if text.startswith("!"): +161 160 | text = text[1::True] +162 161 | if text.startswith("!"): + +FURB188.py:160:5: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice. + | +158 | if text.startswith("!"): +159 | text = text[1::1] +160 | if text.startswith("!"): + | _____^ +161 | | text = text[1::True] + | |____________________________^ FURB188 +162 | if text.startswith("!"): +163 | text = text[1::None] + | + = help: Use removeprefix instead of assignment conditional upon startswith. + +ℹ Safe fix +157 157 | text = "!x!y!z" +158 158 | if text.startswith("!"): +159 159 | text = text[1::1] +160 |- if text.startswith("!"): +161 |- text = text[1::True] + 160 |+ text = text.removeprefix("!") +162 161 | if text.startswith("!"): +163 162 | text = text[1::None] +164 163 | print(text) + +FURB188.py:162:5: FURB188 [*] Prefer `removeprefix` over conditionally replacing with slice. + | +160 | if text.startswith("!"): +161 | text = text[1::True] +162 | if text.startswith("!"): + | _____^ +163 | | text = text[1::None] + | |____________________________^ FURB188 +164 | print(text) + | + = help: Use removeprefix instead of assignment conditional upon startswith. + +ℹ Safe fix +159 159 | text = text[1::1] +160 160 | if text.startswith("!"): +161 161 | text = text[1::True] +162 |- if text.startswith("!"): +163 |- text = text[1::None] + 162 |+ text = text.removeprefix("!") +164 163 | print(text) +165 164 | +166 165 | From 910a3d9d8040791b0c0f43895858ee518dcd3273 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Wed, 18 Sep 2024 23:14:22 -0500 Subject: [PATCH 5/7] change comment wording --- .../refurb/rules/slice_to_remove_prefix_or_suffix.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs index ee0395fdec106b..cc9c0760bfefc2 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs @@ -249,18 +249,18 @@ fn affix_removal_data<'a>( } let slice = slice.as_slice_expr()?; - // Exit early _unless_ slice step is... + // Exit early if slice step is... if slice .step .as_ref() - // omitted + // present and .is_some_and(|step| match step.as_ref() { - // equal to 1 + // not equal to 1 ast::Expr::NumberLiteral(ast::ExprNumberLiteral { range: _, value: ast::Number::Int(x), }) => !(x.as_u8().is_some_and(|val| val == 1)), - // or `None` or `True` + // and not equal to `None` or `True` ast::Expr::NoneLiteral(_) | ast::Expr::BooleanLiteral(ast::ExprBooleanLiteral { range: _, From 68b46bae90359581f1c902ae944564da4e5c7819 Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Thu, 19 Sep 2024 00:01:59 -0500 Subject: [PATCH 6/7] better test for literal 1 --- .../src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs index cc9c0760bfefc2..91c7c9ec795081 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs @@ -259,7 +259,7 @@ fn affix_removal_data<'a>( ast::Expr::NumberLiteral(ast::ExprNumberLiteral { range: _, value: ast::Number::Int(x), - }) => !(x.as_u8().is_some_and(|val| val == 1)), + }) => x.as_u8() != Some(1), // and not equal to `None` or `True` ast::Expr::NoneLiteral(_) | ast::Expr::BooleanLiteral(ast::ExprBooleanLiteral { From b5b12ae4b74d24ad0ac7c10217f86ac5d7852daf Mon Sep 17 00:00:00 2001 From: dylwil3 Date: Thu, 19 Sep 2024 08:03:36 -0500 Subject: [PATCH 7/7] nits --- .../refurb/rules/slice_to_remove_prefix_or_suffix.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs index 91c7c9ec795081..ddd7e0a918a48e 100644 --- a/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs +++ b/crates/ruff_linter/src/rules/refurb/rules/slice_to_remove_prefix_or_suffix.rs @@ -252,20 +252,17 @@ fn affix_removal_data<'a>( // Exit early if slice step is... if slice .step - .as_ref() + .as_deref() // present and - .is_some_and(|step| match step.as_ref() { + .is_some_and(|step| match step { // not equal to 1 ast::Expr::NumberLiteral(ast::ExprNumberLiteral { - range: _, value: ast::Number::Int(x), + .. }) => x.as_u8() != Some(1), // and not equal to `None` or `True` ast::Expr::NoneLiteral(_) - | ast::Expr::BooleanLiteral(ast::ExprBooleanLiteral { - range: _, - value: true, - }) => false, + | ast::Expr::BooleanLiteral(ast::ExprBooleanLiteral { value: true, .. }) => false, _ => true, }) {