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

Support glob patterns for raises_require_match_for and raises_require_match_for #6635

Merged
merged 4 commits into from
Aug 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
@@ -1,3 +1,4 @@
from pickle import PicklingError, UnpicklingError
import socket

import pytest
Expand All @@ -20,6 +21,12 @@ def test_error_no_argument_given():
with pytest.raises(socket.error):
raise ValueError("Can't divide 1 by 0")

with pytest.raises(PicklingError):
raise PicklingError("Can't pickle")

with pytest.raises(UnpicklingError):
raise UnpicklingError("Can't unpickle")


def test_error_match_is_empty():
with pytest.raises(ValueError, match=None):
Expand Down
23 changes: 21 additions & 2 deletions crates/ruff/src/rules/flake8_pytest_style/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod tests {
use test_case::test_case;

use crate::registry::Rule;
use crate::settings::types::IdentifierPattern;
use crate::test::test_path;
use crate::{assert_messages, settings};

Expand Down Expand Up @@ -143,7 +144,7 @@ mod tests {
Rule::PytestRaisesTooBroad,
Path::new("PT011.py"),
Settings {
raises_extend_require_match_for: vec!["ZeroDivisionError".to_string()],
raises_extend_require_match_for: vec![IdentifierPattern::new("ZeroDivisionError").unwrap()],
..Settings::default()
},
"PT011_extend_broad_exceptions"
Expand All @@ -152,11 +153,29 @@ mod tests {
Rule::PytestRaisesTooBroad,
Path::new("PT011.py"),
Settings {
raises_require_match_for: vec!["ZeroDivisionError".to_string()],
raises_require_match_for: vec![IdentifierPattern::new("ZeroDivisionError").unwrap()],
..Settings::default()
},
"PT011_replace_broad_exceptions"
)]
#[test_case(
Rule::PytestRaisesTooBroad,
Path::new("PT011.py"),
Settings {
raises_require_match_for: vec![IdentifierPattern::new("*").unwrap()],
..Settings::default()
},
"PT011_glob_all"
)]
#[test_case(
Rule::PytestRaisesTooBroad,
Path::new("PT011.py"),
Settings {
raises_require_match_for: vec![IdentifierPattern::new("pickle.*").unwrap()],
..Settings::default()
},
"PT011_glob_prefix"
)]
#[test_case(
Rule::PytestRaisesWithMultipleStatements,
Path::new("PT012.py"),
Expand Down
12 changes: 4 additions & 8 deletions crates/ruff/src/rules/flake8_pytest_style/rules/raises.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::call_path::format_call_path;
use ruff_python_ast::call_path::from_qualified_name;
use ruff_python_ast::helpers::is_compound_statement;
use ruff_python_ast::{self as ast, Expr, Ranged, Stmt, WithItem};
use ruff_python_semantic::SemanticModel;
Expand Down Expand Up @@ -231,7 +230,8 @@ fn exception_needs_match(checker: &mut Checker, exception: &Expr) {
.semantic()
.resolve_call_path(exception)
.and_then(|call_path| {
let is_broad_exception = checker
let call_path = format_call_path(&call_path);
checker
.settings
.flake8_pytest_style
.raises_require_match_for
Expand All @@ -242,12 +242,8 @@ fn exception_needs_match(checker: &mut Checker, exception: &Expr) {
.flake8_pytest_style
.raises_extend_require_match_for,
)
.any(|target| call_path == from_qualified_name(target));
if is_broad_exception {
Some(format_call_path(&call_path))
} else {
None
}
.any(|pattern| pattern.matches(&call_path))
.then_some(call_path)
})
{
checker.diagnostics.push(Diagnostic::new(
Expand Down
88 changes: 78 additions & 10 deletions crates/ruff/src/rules/flake8_pytest_style/settings.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
//! Settings for the `flake8-pytest-style` plugin.
use std::error::Error;
use std::fmt;

use serde::{Deserialize, Serialize};

use crate::settings::types::IdentifierPattern;
use ruff_macros::{CacheKey, CombineOptions, ConfigurationOptions};

use super::types;

fn default_broad_exceptions() -> Vec<String> {
fn default_broad_exceptions() -> Vec<IdentifierPattern> {
[
"BaseException",
"Exception",
Expand All @@ -16,7 +19,7 @@ fn default_broad_exceptions() -> Vec<String> {
"EnvironmentError",
"socket.error",
]
.map(ToString::to_string)
.map(|pattern| IdentifierPattern::new(pattern).expect("invalid default exception pattern"))
.to_vec()
}

Expand Down Expand Up @@ -86,6 +89,9 @@ pub struct Options {
)]
/// List of exception names that require a match= parameter in a
/// `pytest.raises()` call.
///
/// Supports glob patterns. For more information on the glob syntax, refer
/// to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax).
pub raises_require_match_for: Option<Vec<String>>,
#[option(
default = "[]",
Expand All @@ -100,6 +106,9 @@ pub struct Options {
/// the entire list.
/// Note that this option does not remove any exceptions from the default
/// list.
///
/// Supports glob patterns. For more information on the glob syntax, refer
/// to the [`globset` documentation](https://docs.rs/globset/latest/globset/#syntax).
pub raises_extend_require_match_for: Option<Vec<String>>,
#[option(
default = "true",
Expand All @@ -120,26 +129,44 @@ pub struct Settings {
pub parametrize_names_type: types::ParametrizeNameType,
pub parametrize_values_type: types::ParametrizeValuesType,
pub parametrize_values_row_type: types::ParametrizeValuesRowType,
pub raises_require_match_for: Vec<String>,
pub raises_extend_require_match_for: Vec<String>,
pub raises_require_match_for: Vec<IdentifierPattern>,
pub raises_extend_require_match_for: Vec<IdentifierPattern>,
pub mark_parentheses: bool,
}

impl From<Options> for Settings {
fn from(options: Options) -> Self {
Self {
impl TryFrom<Options> for Settings {
type Error = SettingsError;

fn try_from(options: Options) -> Result<Self, Self::Error> {
Ok(Self {
fixture_parentheses: options.fixture_parentheses.unwrap_or(true),
parametrize_names_type: options.parametrize_names_type.unwrap_or_default(),
parametrize_values_type: options.parametrize_values_type.unwrap_or_default(),
parametrize_values_row_type: options.parametrize_values_row_type.unwrap_or_default(),
raises_require_match_for: options
.raises_require_match_for
.map(|patterns| {
patterns
.into_iter()
.map(|pattern| IdentifierPattern::new(&pattern))
.collect()
})
.transpose()
.map_err(SettingsError::InvalidRaisesRequireMatchFor)?
.unwrap_or_else(default_broad_exceptions),
raises_extend_require_match_for: options
.raises_extend_require_match_for
.map(|patterns| {
patterns
.into_iter()
.map(|pattern| IdentifierPattern::new(&pattern))
.collect()
})
.transpose()
.map_err(SettingsError::InvalidRaisesExtendRequireMatchFor)?
.unwrap_or_default(),
mark_parentheses: options.mark_parentheses.unwrap_or(true),
}
})
}
}
impl From<Settings> for Options {
Expand All @@ -149,8 +176,20 @@ impl From<Settings> for Options {
parametrize_names_type: Some(settings.parametrize_names_type),
parametrize_values_type: Some(settings.parametrize_values_type),
parametrize_values_row_type: Some(settings.parametrize_values_row_type),
raises_require_match_for: Some(settings.raises_require_match_for),
raises_extend_require_match_for: Some(settings.raises_extend_require_match_for),
raises_require_match_for: Some(
settings
.raises_require_match_for
.iter()
.map(ToString::to_string)
.collect(),
),
raises_extend_require_match_for: Some(
settings
.raises_extend_require_match_for
.iter()
.map(ToString::to_string)
.collect(),
),
mark_parentheses: Some(settings.mark_parentheses),
}
}
Expand All @@ -169,3 +208,32 @@ impl Default for Settings {
}
}
}

/// Error returned by the [`TryFrom`] implementation of [`Settings`].
#[derive(Debug)]
pub enum SettingsError {
InvalidRaisesRequireMatchFor(glob::PatternError),
InvalidRaisesExtendRequireMatchFor(glob::PatternError),
}

impl fmt::Display for SettingsError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
SettingsError::InvalidRaisesRequireMatchFor(err) => {
write!(f, "invalid raises-require-match-for pattern: {err}")
}
SettingsError::InvalidRaisesExtendRequireMatchFor(err) => {
write!(f, "invalid raises-extend-require-match-for pattern: {err}")
}
}
}
}

impl Error for SettingsError {
fn source(&self) -> Option<&(dyn Error + 'static)> {
match self {
SettingsError::InvalidRaisesRequireMatchFor(err) => Some(err),
SettingsError::InvalidRaisesExtendRequireMatchFor(err) => Some(err),
}
}
}
Original file line number Diff line number Diff line change
@@ -1,47 +1,47 @@
---
source: crates/ruff/src/rules/flake8_pytest_style/mod.rs
---
PT011.py:17:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
PT011.py:18:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
|
16 | def test_error_no_argument_given():
17 | with pytest.raises(ValueError):
17 | def test_error_no_argument_given():
18 | with pytest.raises(ValueError):
| ^^^^^^^^^^ PT011
18 | raise ValueError("Can't divide 1 by 0")
19 | raise ValueError("Can't divide 1 by 0")
|

PT011.py:20:24: PT011 `pytest.raises(socket.error)` is too broad, set the `match` parameter or use a more specific exception
PT011.py:21:24: PT011 `pytest.raises(socket.error)` is too broad, set the `match` parameter or use a more specific exception
|
18 | raise ValueError("Can't divide 1 by 0")
19 |
20 | with pytest.raises(socket.error):
19 | raise ValueError("Can't divide 1 by 0")
20 |
21 | with pytest.raises(socket.error):
| ^^^^^^^^^^^^ PT011
21 | raise ValueError("Can't divide 1 by 0")
22 | raise ValueError("Can't divide 1 by 0")
|

PT011.py:25:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
PT011.py:32:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
|
24 | def test_error_match_is_empty():
25 | with pytest.raises(ValueError, match=None):
31 | def test_error_match_is_empty():
32 | with pytest.raises(ValueError, match=None):
| ^^^^^^^^^^ PT011
26 | raise ValueError("Can't divide 1 by 0")
33 | raise ValueError("Can't divide 1 by 0")
|

PT011.py:28:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
PT011.py:35:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
|
26 | raise ValueError("Can't divide 1 by 0")
27 |
28 | with pytest.raises(ValueError, match=""):
33 | raise ValueError("Can't divide 1 by 0")
34 |
35 | with pytest.raises(ValueError, match=""):
| ^^^^^^^^^^ PT011
29 | raise ValueError("Can't divide 1 by 0")
36 | raise ValueError("Can't divide 1 by 0")
|

PT011.py:31:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
PT011.py:38:24: PT011 `pytest.raises(ValueError)` is too broad, set the `match` parameter or use a more specific exception
|
29 | raise ValueError("Can't divide 1 by 0")
30 |
31 | with pytest.raises(ValueError, match=f""):
36 | raise ValueError("Can't divide 1 by 0")
37 |
38 | with pytest.raises(ValueError, match=f""):
| ^^^^^^^^^^ PT011
32 | raise ValueError("Can't divide 1 by 0")
39 | raise ValueError("Can't divide 1 by 0")
|


Loading
Loading