Skip to content

Commit

Permalink
Omit NotImplementedError from TRY003 (#6568)
Browse files Browse the repository at this point in the history
Closes #6528.
  • Loading branch information
charliermarsh authored Aug 14, 2023
1 parent 96d310f commit 4686247
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 25 deletions.
4 changes: 4 additions & 0 deletions crates/ruff/resources/test/fixtures/tryceratops/TRY003.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,7 @@ def good(a: int):
def another_good(a):
if a % 2 == 0:
raise GoodArgCantBeEven(a)


def another_good():
raise NotImplementedError("This is acceptable too")
66 changes: 41 additions & 25 deletions crates/ruff/src/rules/tryceratops/rules/raise_vanilla_args.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Ranged};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Arguments, Constant, Expr, Ranged};

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

Expand All @@ -17,6 +16,10 @@ use crate::checkers::ast::Checker;
/// If the exception message is instead defined within the exception class, it
/// will be consistent across all `raise` invocations.
///
/// This rule is not enforced for some built-in exceptions that are commonly
/// raised with a message and would be unusual to subclass, such as
/// `NotImplementedError`.
///
/// ## Example
/// ```python
/// class CantBeNegative(Exception):
Expand Down Expand Up @@ -49,14 +52,44 @@ impl Violation for RaiseVanillaArgs {
}
}

fn any_string<F>(expr: &Expr, predicate: F) -> bool
where
F: (Fn(&str) -> bool) + Copy,
{
/// TRY003
pub(crate) fn raise_vanilla_args(checker: &mut Checker, expr: &Expr) {
let Expr::Call(ast::ExprCall {
func,
arguments: Arguments { args, .. },
..
}) = expr else {
return;
};

let Some(arg) = args.first() else {
return;
};

// Ignore some built-in exceptions that don't make sense to subclass, like
// `NotImplementedError`.
if checker
.semantic()
.resolve_call_path(func)
.is_some_and(|call_path| matches!(call_path.as_slice(), ["", "NotImplementedError"]))
{
return;
}

if contains_message(arg) {
checker
.diagnostics
.push(Diagnostic::new(RaiseVanillaArgs, expr.range()));
}
}

/// Returns `true` if an expression appears to be an exception message (i.e., a string with
/// some whitespace).
fn contains_message(expr: &Expr) -> bool {
match expr {
Expr::FString(ast::ExprFString { values, .. }) => {
for value in values {
if any_string(value, predicate) {
if contains_message(value) {
return true;
}
}
Expand All @@ -65,7 +98,7 @@ where
value: Constant::Str(value),
..
}) => {
if predicate(value) {
if value.chars().any(char::is_whitespace) {
return true;
}
}
Expand All @@ -74,20 +107,3 @@ where

false
}

/// TRY003
pub(crate) fn raise_vanilla_args(checker: &mut Checker, expr: &Expr) {
if let Expr::Call(ast::ExprCall {
arguments: Arguments { args, .. },
..
}) = expr
{
if let Some(arg) = args.first() {
if any_string(arg, |part| part.chars().any(char::is_whitespace)) {
checker
.diagnostics
.push(Diagnostic::new(RaiseVanillaArgs, expr.range()));
}
}
}
}

0 comments on commit 4686247

Please sign in to comment.