Skip to content

Commit

Permalink
Tweak message, add more tests
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Apr 6, 2024
1 parent 0069b84 commit fe94b89
Show file tree
Hide file tree
Showing 8 changed files with 236 additions and 128 deletions.
22 changes: 22 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/refurb/FURB110.py
Expand Up @@ -6,3 +6,25 @@
z = x if x \
else \
y # FURB110

z = x() if x() else y() # FURB110

# FURB110
z = x if (
# Test for x.
x
) else (
# Test for y.
y
)

# FURB110
z = (
x if (
# Test for x.
x
) else (
# Test for y.
y
)
)
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Expand Up @@ -1363,8 +1363,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::IfExprMinMax) {
refurb::rules::if_expr_min_max(checker, if_exp);
}
if checker.enabled(Rule::OrOper) {
refurb::rules::or_oper(checker, if_exp);
if checker.enabled(Rule::IfExpInsteadOfOr) {
refurb::rules::if_exp_instead_of_or(checker, if_exp);
}
}
Expr::ListComp(
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Expand Up @@ -1037,7 +1037,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
// refurb
(Refurb, "101") => (RuleGroup::Preview, rules::refurb::rules::ReadWholeFile),
(Refurb, "105") => (RuleGroup::Preview, rules::refurb::rules::PrintEmptyString),
(Refurb, "110") => (RuleGroup::Preview, rules::refurb::rules::OrOper),
(Refurb, "110") => (RuleGroup::Preview, rules::refurb::rules::IfExpInsteadOfOr),
#[allow(deprecated)]
(Refurb, "113") => (RuleGroup::Nursery, rules::refurb::rules::RepeatedAppend),
(Refurb, "118") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedOperator),
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/rules/refurb/mod.rs
Expand Up @@ -16,7 +16,7 @@ mod tests {

#[test_case(Rule::ReadWholeFile, Path::new("FURB101.py"))]
#[test_case(Rule::RepeatedAppend, Path::new("FURB113.py"))]
#[test_case(Rule::OrOper, Path::new("FURB110.py"))]
#[test_case(Rule::IfExpInsteadOfOr, Path::new("FURB110.py"))]
#[test_case(Rule::ReimplementedOperator, Path::new("FURB118.py"))]
#[test_case(Rule::ReadlinesInFor, Path::new("FURB129.py"))]
#[test_case(Rule::DeleteFullSlice, Path::new("FURB131.py"))]
Expand Down
100 changes: 100 additions & 0 deletions crates/ruff_linter/src/rules/refurb/rules/if_exp_instead_of_or.rs
@@ -0,0 +1,100 @@
use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::contains_effect;
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_text_size::Ranged;

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

/// ## What it does
/// Checks for ternary `if` expressions that can be replaced with the `or`
/// operator.
///
/// ## Why is this bad?
/// Ternary `if` expressions are more verbose than `or` expressions while
/// providing the same functionality.
///
/// ## Example
/// ```python
/// z = x if x else y
/// ```
///
/// Use instead:
/// ```python
/// z = x or y
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe in the event that the body of the
/// `if` expression contains side effects.
///
/// For example, `foo` will be called twice in `foo() if foo() else bar()`
/// (assuming it returns a truthy value), but only once in `foo() or bar()`.
#[violation]
pub struct IfExpInsteadOfOr;

impl Violation for IfExpInsteadOfOr {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Replace ternary `if` expression with `or` operator")
}

fn fix_title(&self) -> Option<String> {
Some(format!("Replace with `or` operator"))
}
}

/// FURB110
pub(crate) fn if_exp_instead_of_or(checker: &mut Checker, if_expr: &ast::ExprIf) {
let ast::ExprIf {
test,
body,
orelse,
range,
} = if_expr;

if ComparableExpr::from(test) != ComparableExpr::from(body) {
return;
}

let mut diagnostic = Diagnostic::new(IfExpInsteadOfOr, *range);

// Tokenize from the end of the `body`.
let left = parenthesized_range(
test.into(),
if_expr.into(),
checker.indexer().comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(test.range());

let right = parenthesized_range(
orelse.into(),
if_expr.into(),
checker.indexer().comment_ranges(),
checker.locator().contents(),
)
.unwrap_or(orelse.range());

diagnostic.set_fix(Fix::applicable_edit(
Edit::range_replacement(
format!(
"{} or {}",
checker.locator().slice(left),
checker.locator().slice(right),
),
if_expr.range(),
),
if contains_effect(&body, |id| checker.semantic().is_builtin(id)) {
Applicability::Unsafe
} else {
Applicability::Safe
},
));

checker.diagnostics.push(diagnostic);
}
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/refurb/rules/mod.rs
Expand Up @@ -3,14 +3,14 @@ pub(crate) use check_and_remove_from_set::*;
pub(crate) use delete_full_slice::*;
pub(crate) use for_loop_set_mutations::*;
pub(crate) use hashlib_digest_hex::*;
pub(crate) use if_exp_instead_of_or::*;
pub(crate) use if_expr_min_max::*;
pub(crate) use implicit_cwd::*;
pub(crate) use int_on_sliced_str::*;
pub(crate) use isinstance_type_none::*;
pub(crate) use list_reverse_copy::*;
pub(crate) use math_constant::*;
pub(crate) use metaclass_abcmeta::*;
pub(crate) use or_oper::*;
pub(crate) use print_empty_string::*;
pub(crate) use read_whole_file::*;
pub(crate) use readlines_in_for::*;
Expand All @@ -31,14 +31,14 @@ mod check_and_remove_from_set;
mod delete_full_slice;
mod for_loop_set_mutations;
mod hashlib_digest_hex;
mod if_exp_instead_of_or;
mod if_expr_min_max;
mod implicit_cwd;
mod int_on_sliced_str;
mod isinstance_type_none;
mod list_reverse_copy;
mod math_constant;
mod metaclass_abcmeta;
mod or_oper;
mod print_empty_string;
mod read_whole_file;
mod readlines_in_for;
Expand Down
101 changes: 0 additions & 101 deletions crates/ruff_linter/src/rules/refurb/rules/or_oper.rs

This file was deleted.

0 comments on commit fe94b89

Please sign in to comment.