Omit NotImplementedError from TRY003 (#6568)

Closes https://github.com/astral-sh/ruff/issues/6528.
This commit is contained in:
Charlie Marsh 2023-08-14 14:24:44 -04:00 committed by GitHub
parent 96d310fbab
commit 46862473b9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 45 additions and 25 deletions

View file

@ -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")

View file

@ -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;
@ -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):
@ -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;
}
}
@ -65,7 +98,7 @@ where
value: Constant::Str(value),
..
}) => {
if predicate(value) {
if value.chars().any(char::is_whitespace) {
return true;
}
}
@ -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()));
}
}
}
}