Skip to content

Commit

Permalink
[ruff-0.8] Spruce up docs for newly stabilised rules (#14466)
Browse files Browse the repository at this point in the history
## Summary

- Expand some docs where they're unclear about the motivation, or assume
some knowledge that hasn't been introduced yet
- Add more links to external docs
- Rename PYI063 from `PrePep570PositionalArgument` to
`Pep484StylePositionalOnlyParameter`
- Rename the file `parenthesize_logical_operators.rs` to
`parenthesize_chained_operators.rs`, since the rule is called
`ParenthesizeChainedOperators`, not `ParenthesizeLogicalOperators`

## Test Plan

`cargo test`
  • Loading branch information
AlexWaygood authored and MichaReiser committed Nov 20, 2024
1 parent b4858a9 commit b61eda2
Show file tree
Hide file tree
Showing 17 changed files with 124 additions and 101 deletions.
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,8 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::RedundantNumericUnion) {
flake8_pyi::rules::redundant_numeric_union(checker, parameters);
}
if checker.enabled(Rule::PrePep570PositionalArgument) {
flake8_pyi::rules::pre_pep570_positional_argument(checker, function_def);
if checker.enabled(Rule::Pep484StylePositionalOnlyParameter) {
flake8_pyi::rules::pep_484_positional_parameter(checker, function_def);
}
if checker.enabled(Rule::DunderFunctionName) {
if let Some(diagnostic) = pep8_naming::rules::dunder_function_name(
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass),
(Flake8Pyi, "061") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantNoneLiteral),
(Flake8Pyi, "062") => (RuleGroup::Stable, rules::flake8_pyi::rules::DuplicateLiteralMember),
(Flake8Pyi, "063") => (RuleGroup::Stable, rules::flake8_pyi::rules::PrePep570PositionalArgument),
(Flake8Pyi, "063") => (RuleGroup::Stable, rules::flake8_pyi::rules::Pep484StylePositionalOnlyParameter),
(Flake8Pyi, "064") => (RuleGroup::Stable, rules::flake8_pyi::rules::RedundantFinalLiteral),
(Flake8Pyi, "066") => (RuleGroup::Stable, rules::flake8_pyi::rules::BadVersionInfoOrder),

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,14 @@ use crate::rules::fastapi::rules::is_fastapi_route;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Identifies FastAPI routes with deprecated uses of `Depends`.
/// Identifies FastAPI routes with deprecated uses of `Depends` or similar.
///
/// ## Why is this bad?
/// The FastAPI documentation recommends the use of `Annotated` for defining
/// route dependencies and parameters, rather than using `Depends` directly
/// with a default value.
///
/// This approach is also suggested for various route parameters, including Body and Cookie, as it helps ensure consistency and clarity in defining dependencies and parameters.
/// The [FastAPI documentation] recommends the use of [`typing.Annotated`] for
/// defining route dependencies and parameters, rather than using `Depends`,
/// `Query` or similar as a default value for a parameter. Using this approach
/// everywhere helps ensure consistency and clarity in defining dependencies
/// and parameters.
///
/// ## Example
///
Expand Down Expand Up @@ -55,7 +55,9 @@ use crate::settings::types::PythonVersion;
/// async def read_items(commons: Annotated[dict, Depends(common_parameters)]):
/// return commons
/// ```
///
/// [fastAPI documentation]: https://fastapi.tiangolo.com/tutorial/query-params-str-validations/?h=annotated#advantages-of-annotated
/// [typing.Annotated]: https://docs.python.org/3/library/typing.html#typing.Annotated
#[violation]
pub struct FastApiNonAnnotatedDependency;

Expand All @@ -72,7 +74,7 @@ impl Violation for FastApiNonAnnotatedDependency {
}
}

/// RUF103
/// FAST002
pub(crate) fn fastapi_non_annotated_dependency(
checker: &mut Checker,
function_def: &ast::StmtFunctionDef,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl AlwaysFixableViolation for FastApiRedundantResponseModel {
}
}

/// RUF102
/// FAST001
pub(crate) fn fastapi_redundant_response_model(
checker: &mut Checker,
function_def: &StmtFunctionDef,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ use crate::checkers::ast::Checker;
/// the `ContextVar`. If the object is modified, those modifications will persist
/// across calls, which can lead to unexpected behavior.
///
/// Instead, prefer to use immutable data structures; or, take `None` as a
/// default, and initialize a new mutable object inside for each call using the
/// `.set()` method.
/// Instead, prefer to use immutable data structures. Alternatively, take
/// `None` as a default, and initialize a new mutable object inside for each
/// call using the `.set()` method.
///
/// Types outside the standard library can be marked as immutable with the
/// [`lint.flake8-bugbear.extend-immutable-calls`] configuration option.
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ mod tests {
#[test_case(Rule::PatchVersionComparison, Path::new("PYI004.pyi"))]
#[test_case(Rule::QuotedAnnotationInStub, Path::new("PYI020.py"))]
#[test_case(Rule::QuotedAnnotationInStub, Path::new("PYI020.pyi"))]
#[test_case(Rule::PrePep570PositionalArgument, Path::new("PYI063.py"))]
#[test_case(Rule::PrePep570PositionalArgument, Path::new("PYI063.pyi"))]
#[test_case(Rule::Pep484StylePositionalOnlyParameter, Path::new("PYI063.py"))]
#[test_case(Rule::Pep484StylePositionalOnlyParameter, Path::new("PYI063.pyi"))]
#[test_case(Rule::RedundantFinalLiteral, Path::new("PYI064.py"))]
#[test_case(Rule::RedundantFinalLiteral, Path::new("PYI064.pyi"))]
#[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.py"))]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ use crate::registry::Rule;
/// Comparing `sys.version_info` with `==` or `<=` has unexpected behavior
/// and can lead to bugs.
///
/// For example, `sys.version_info > (3, 8)` will also match `3.8.10`,
/// while `sys.version_info <= (3, 8)` will _not_ match `3.8.10`:
/// For example, `sys.version_info > (3, 8, 1)` will resolve to `True` if your
/// Python version is 3.8.1; meanwhile, `sys.version_info <= (3, 8)` will _not_
/// resolve to `True` if your Python version is 3.8.10:
///
/// ```python
/// >>> import sys
Expand Down Expand Up @@ -61,16 +62,20 @@ impl Violation for BadVersionInfoComparison {
}

/// ## What it does
/// Checks for if-else statements with `sys.version_info` comparisons that use
/// `<` comparators.
/// Checks for code that branches on `sys.version_info` comparisons where
/// branches corresponding to older Python versions come before branches
/// corresponding to newer Python versions.
///
/// ## Why is this bad?
/// As a convention, branches that correspond to newer Python versions should
/// come first when using `sys.version_info` comparisons. This makes it easier
/// to understand the desired behavior, which typically corresponds to the
/// latest Python versions.
/// come first. This makes it easier to understand the desired behavior, which
/// typically corresponds to the latest Python versions.
///
/// In [preview], this rule will also flag non-stub files.
/// This rule enforces the convention by checking for `if` tests that compare
/// `sys.version_info` with `<` rather than `>=`.
///
/// By default, this rule only applies to stub files.
/// In [preview], it will also flag this anti-pattern in non-stub files.
///
/// ## Example
///
Expand Down Expand Up @@ -101,7 +106,7 @@ pub struct BadVersionInfoOrder;
impl Violation for BadVersionInfoOrder {
#[derive_message_formats]
fn message(&self) -> String {
"Use `>=` when using `if`-`else` with `sys.version_info` comparisons".to_string()
"Put branches for newer Python versions first when branching on `sys.version_info` comparisons".to_string()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,19 @@ use crate::checkers::ast::Checker;
use crate::settings::types::PythonVersion;

/// ## What it does
/// Checks for the presence of [PEP 484]-style positional-only arguments.
/// Checks for the presence of [PEP 484]-style positional-only parameters.
///
/// ## Why is this bad?
/// Historically, [PEP 484] recommended prefixing positional-only arguments
/// with a double underscore (`__`). However, [PEP 570] introduced a dedicated
/// syntax for positional-only arguments, which should be preferred.
/// Historically, [PEP 484] recommended prefixing parameter names with double
/// underscores (`__`) to indicate to a type checker that they were
/// positional-only. However, [PEP 570] (introduced in Python 3.8) introduced
/// dedicated syntax for positional-only arguments. If a forward slash (`/`) is
/// present in a function signature on Python 3.8+, all parameters prior to the
/// slash are interpreted as positional-only.
///
/// The new syntax should be preferred as it is more widely used, more concise
/// and more readable. It is also respected by Python at runtime, whereas the
/// old-style syntax was only understood by type checkers.
///
/// ## Example
///
Expand All @@ -33,12 +40,12 @@ use crate::settings::types::PythonVersion;
/// [PEP 484]: https://peps.python.org/pep-0484/#positional-only-arguments
/// [PEP 570]: https://peps.python.org/pep-0570
#[violation]
pub struct PrePep570PositionalArgument;
pub struct Pep484StylePositionalOnlyParameter;

impl Violation for PrePep570PositionalArgument {
impl Violation for Pep484StylePositionalOnlyParameter {
#[derive_message_formats]
fn message(&self) -> String {
"Use PEP 570 syntax for positional-only arguments".to_string()
"Use PEP 570 syntax for positional-only parameters".to_string()
}

fn fix_title(&self) -> Option<String> {
Expand All @@ -47,7 +54,7 @@ impl Violation for PrePep570PositionalArgument {
}

/// PYI063
pub(crate) fn pre_pep570_positional_argument(
pub(crate) fn pep_484_positional_parameter(
checker: &mut Checker,
function_def: &ast::StmtFunctionDef,
) {
Expand Down Expand Up @@ -82,18 +89,18 @@ pub(crate) fn pre_pep570_positional_argument(
));

if let Some(arg) = function_def.parameters.args.get(skip) {
if is_pre_pep570_positional_only(arg) {
if is_old_style_positional_only(arg) {
checker.diagnostics.push(Diagnostic::new(
PrePep570PositionalArgument,
Pep484StylePositionalOnlyParameter,
arg.identifier(),
));
}
}
}

/// Returns `true` if the [`ParameterWithDefault`] is an old-style positional-only argument (i.e.,
/// Returns `true` if the [`ParameterWithDefault`] is an old-style positional-only parameter (i.e.,
/// its name starts with `__` and does not end with `__`).
fn is_pre_pep570_positional_only(arg: &ParameterWithDefault) -> bool {
fn is_old_style_positional_only(arg: &ParameterWithDefault) -> bool {
let arg_name = &arg.parameter.name;
arg_name.starts_with("__") && !arg_name.ends_with("__")
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,25 @@ use crate::Locator;
/// Checks for redundant `Final[Literal[...]]` annotations.
///
/// ## Why is this bad?
/// A `Final[Literal[...]]` annotation can be replaced with `Final`; the literal
/// use is unnecessary.
/// All constant variables annotated as `Final` are understood as implicitly
/// having `Literal` types by a type checker. As such, a `Final[Literal[...]]`
/// annotation can often be replaced with a bare `Final`, annotation, which
/// will have the same meaning to the type checker while being more concise and
/// more readable.
///
/// ## Example
///
/// ```pyi
/// from typing import Final, Literal
///
/// x: Final[Literal[42]]
/// y: Final[Literal[42]] = 42
/// ```
///
/// Use instead:
/// ```pyi
/// from typing import Final, Literal
///
/// x: Final = 42
/// y: Final = 42
/// ```
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
---
source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
snapshot_kind: text
---
PYI063.py:6:9: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:6:9: PYI063 Use PEP 570 syntax for positional-only parameters
|
4 | from typing import Self
5 |
Expand All @@ -13,7 +12,7 @@ PYI063.py:6:9: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:7:14: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:7:14: PYI063 Use PEP 570 syntax for positional-only parameters
|
6 | def bad(__x: int) -> None: ... # PYI063
7 | def also_bad(__x: int, __y: str) -> None: ... # PYI063
Expand All @@ -22,7 +21,7 @@ PYI063.py:7:14: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:8:15: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:8:15: PYI063 Use PEP 570 syntax for positional-only parameters
|
6 | def bad(__x: int) -> None: ... # PYI063
7 | def also_bad(__x: int, __y: str) -> None: ... # PYI063
Expand All @@ -33,7 +32,7 @@ PYI063.py:8:15: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:24:14: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:24:14: PYI063 Use PEP 570 syntax for positional-only parameters
|
22 | def bad(__self) -> None: ... # PYI063
23 | @staticmethod
Expand All @@ -44,7 +43,7 @@ PYI063.py:24:14: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:25:22: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:25:22: PYI063 Use PEP 570 syntax for positional-only parameters
|
23 | @staticmethod
24 | def bad2(__self) -> None: ... # PYI063
Expand All @@ -55,7 +54,7 @@ PYI063.py:25:22: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:26:25: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:26:25: PYI063 Use PEP 570 syntax for positional-only parameters
|
24 | def bad2(__self) -> None: ... # PYI063
25 | def bad3(__self, __x: int) -> None: ... # PYI063
Expand All @@ -66,7 +65,7 @@ PYI063.py:26:25: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:28:25: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:28:25: PYI063 Use PEP 570 syntax for positional-only parameters
|
26 | def still_bad(self, __x_: int) -> None: ... # PYI063
27 | @staticmethod
Expand All @@ -77,7 +76,7 @@ PYI063.py:28:25: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:30:23: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:30:23: PYI063 Use PEP 570 syntax for positional-only parameters
|
28 | def this_is_bad_too(__x: int) -> None: ... # PYI063
29 | @classmethod
Expand All @@ -88,7 +87,7 @@ PYI063.py:30:23: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:52:23: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:52:23: PYI063 Use PEP 570 syntax for positional-only parameters
|
50 | class Metaclass(type):
51 | @classmethod
Expand All @@ -99,7 +98,7 @@ PYI063.py:52:23: PYI063 Use PEP 570 syntax for positional-only arguments
|
= help: Add `/` to function signature

PYI063.py:56:26: PYI063 Use PEP 570 syntax for positional-only arguments
PYI063.py:56:26: PYI063 Use PEP 570 syntax for positional-only parameters
|
54 | class Metaclass2(type):
55 | @classmethod
Expand Down
Loading

0 comments on commit b61eda2

Please sign in to comment.