From ac150b9314c0359c499aff103a99007807f9f854 Mon Sep 17 00:00:00 2001
From: Alex Waygood <Alex.Waygood@Gmail.com>
Date: Thu, 21 Mar 2024 11:54:43 +0000
Subject: [PATCH] Spruce up docs for flake8-pyi rules (part 2) (#10494)

- Improve clarity over the motivation for some rules
- Improve links to external references. In particular, reduce links to PEPs, as PEPs are generally historical documents rather than pieces of living documentation. Where possible, it's better to link to the official typing spec, the other docs at typing.readthedocs.io/en/latest, or the docs at docs.python.org/3/library/typing.html.
- Use more concise language in a few places
---
 .../rules/flake8_pyi/rules/simple_defaults.rs | 26 +++++++++----------
 .../rules/string_or_bytes_too_long.rs         | 11 +++++---
 .../flake8_pyi/rules/type_alias_naming.rs     |  3 +++
 .../rules/unnecessary_literal_union.rs        | 11 +++++---
 .../rules/unnecessary_type_union.rs           |  8 +++---
 .../flake8_pyi/rules/unrecognized_platform.rs |  9 ++++---
 .../rules/unrecognized_version_info.rs        |  9 +++++++
 .../rules/unsupported_method_call_on_all.rs   | 20 +++++++++++---
 .../rules/unused_private_type_definition.rs   | 10 +++----
 9 files changed, 69 insertions(+), 38 deletions(-)

diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs
index fdf868aba09ac..70102ab892c9d 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs
@@ -17,32 +17,30 @@ use crate::rules::flake8_pyi::rules::TypingModule;
 use crate::settings::types::PythonVersion;
 
 /// ## What it does
-/// Checks for typed function arguments in stubs with default values that
-/// are not "simple" /// (i.e., `int`, `float`, `complex`, `bytes`, `str`,
-/// `bool`, `None`, `...`, or simple container literals).
+/// Checks for typed function arguments in stubs with complex default values.
 ///
 /// ## Why is this bad?
-/// Stub (`.pyi`) files exist to define type hints, and are not evaluated at
-/// runtime. As such, function arguments in stub files should not have default
-/// values, as they are ignored by type checkers.
-///
-/// However, the use of default values may be useful for IDEs and other
-/// consumers of stub files, and so "simple" values may be worth including and
-/// are permitted by this rule.
+/// Stub (`.pyi`) files exist as "data files" for static analysis tools, and
+/// are not evaluated at runtime. While simple default values may be useful for
+/// some tools that consume stubs, such as IDEs, they are ignored by type
+/// checkers.
 ///
 /// Instead of including and reproducing a complex value, use `...` to indicate
-/// that the assignment has a default value, but that the value is non-simple
-/// or varies according to the current platform or Python version.
+/// that the assignment has a default value, but that the value is "complex" or
+/// varies according to the current platform or Python version. For the
+/// purposes of this rule, any default value counts as "complex" unless it is
+/// a literal `int`, `float`, `complex`, `bytes`, `str`, `bool`, `None`, `...`,
+/// or a simple container literal.
 ///
 /// ## Example
 /// ```python
-/// def foo(arg: List[int] = []) -> None:
+/// def foo(arg: list[int] = list(range(10_000))) -> None:
 ///     ...
 /// ```
 ///
 /// Use instead:
 /// ```python
-/// def foo(arg: List[int] = ...) -> None:
+/// def foo(arg: list[int] = ...) -> None:
 ///     ...
 /// ```
 ///
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs
index 145d38f6da6b4..277b07c5e375d 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs
@@ -12,11 +12,14 @@ use crate::checkers::ast::Checker;
 /// in stub (`.pyi`) files.
 ///
 /// ## Why is this bad?
-/// If a function has a default value where the string or bytes representation
-/// is greater than 50 characters, it is likely to be an implementation detail
-/// or a constant that varies depending on the system you're running on.
+/// If a function or variable has a default value where the string or bytes
+/// representation is greater than 50 characters long, it is likely to be an
+/// implementation detail or a constant that varies depending on the system
+/// you're running on.
 ///
-/// Consider replacing such constants with ellipses (`...`).
+/// Although IDEs may find them useful, default values are ignored by type
+/// checkers, the primary consumers of stub files. Replace very long constants
+/// with ellipses (`...`) to simplify the stub.
 ///
 /// ## Example
 /// ```python
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs
index 4e60317211c2c..d9b47cb35ac22 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs
@@ -57,6 +57,9 @@ impl Violation for SnakeCaseTypeAlias {
 ///
 /// _MyType: TypeAlias = int
 /// ```
+///
+/// ## References
+/// - [PEP 484: Type Aliases](https://peps.python.org/pep-0484/#type-aliases)
 #[violation]
 pub struct TSuffixedTypeAlias {
     name: String,
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_literal_union.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_literal_union.rs
index 146e43f26b390..af40e2f6bae44 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_literal_union.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_literal_union.rs
@@ -11,22 +11,25 @@ use crate::checkers::ast::Checker;
 /// Checks for the presence of multiple literal types in a union.
 ///
 /// ## Why is this bad?
-/// Literal types accept multiple arguments, and it is clearer to specify them
-/// as a single literal.
+/// `Literal["foo", 42]` has identical semantics to
+/// `Literal["foo"] | Literal[42]`, but is clearer and more concise.
 ///
 /// ## Example
 /// ```python
 /// from typing import Literal
 ///
-/// field: Literal[1] | Literal[2]
+/// field: Literal[1] | Literal[2] | str
 /// ```
 ///
 /// Use instead:
 /// ```python
 /// from typing import Literal
 ///
-/// field: Literal[1, 2]
+/// field: Literal[1, 2] | str
 /// ```
+///
+/// ## References
+/// - [Python documentation: `typing.Literal`](https://docs.python.org/3/library/typing.html#typing.Literal)
 #[violation]
 pub struct UnnecessaryLiteralUnion {
     members: Vec<String>,
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs
index 0a29b9ad28694..4f1ba2be98553 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs
@@ -12,17 +12,17 @@ use crate::checkers::ast::Checker;
 /// Checks for the presence of multiple `type`s in a union.
 ///
 /// ## Why is this bad?
-/// The `type` built-in function accepts unions, and it is clearer to
-/// explicitly specify them as a single `type`.
+/// `type[T | S]` has identical semantics to `type[T] | type[S]` in a type
+/// annotation, but is cleaner and more concise.
 ///
 /// ## Example
 /// ```python
-/// field: type[int] | type[float]
+/// field: type[int] | type[float] | str
 /// ```
 ///
 /// Use instead:
 /// ```python
-/// field: type[int | float]
+/// field: type[int | float] | str
 /// ```
 #[violation]
 pub struct UnnecessaryTypeUnion {
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_platform.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_platform.rs
index 3565e5d6a8fb2..e10d70d5e8374 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_platform.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_platform.rs
@@ -15,8 +15,9 @@ use crate::registry::Rule;
 ///
 /// ## Why is this bad?
 /// Some `sys.platform` checks are too complex for type checkers to
-/// understand, and thus result in false positives. `sys.platform` checks
-/// should be simple string comparisons, like `sys.platform == "linux"`.
+/// understand, and thus result in incorrect inferences by these tools.
+/// `sys.platform` checks should be simple string comparisons, like
+/// `if sys.platform == "linux"`.
 ///
 /// ## Example
 /// ```python
@@ -39,7 +40,7 @@ use crate::registry::Rule;
 /// ```
 ///
 /// ## References
-/// - [PEP 484](https://peps.python.org/pep-0484/#version-and-platform-checking)
+/// - [Typing stubs documentation: Version and Platform Checks](https://typing.readthedocs.io/en/latest/source/stubs.html#version-and-platform-checks)
 #[violation]
 pub struct UnrecognizedPlatformCheck;
 
@@ -75,7 +76,7 @@ impl Violation for UnrecognizedPlatformCheck {
 /// ```
 ///
 /// ## References
-/// - [PEP 484](https://peps.python.org/pep-0484/#version-and-platform-checking)
+/// - [Typing stubs documentation: Version and Platform Checks](https://typing.readthedocs.io/en/latest/source/stubs.html#version-and-platform-checks)
 #[violation]
 pub struct UnrecognizedPlatformName {
     platform: String,
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_version_info.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_version_info.rs
index 7bc2180afe5f9..aad888aafb82b 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_version_info.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_version_info.rs
@@ -31,6 +31,9 @@ use crate::registry::Rule;
 /// if sys.version_info[0] == 2:
 ///     ...
 /// ```
+///
+/// ## References
+/// - [Typing stubs documentation: Version and Platform Checks](https://typing.readthedocs.io/en/latest/source/stubs.html#version-and-platform-checks)
 #[violation]
 pub struct UnrecognizedVersionInfoCheck;
 
@@ -69,6 +72,9 @@ impl Violation for UnrecognizedVersionInfoCheck {
 /// if sys.version_info >= (3, 4):
 ///     ...
 /// ```
+///
+/// ## References
+/// - [Typing stubs documentation: Version and Platform Checks](https://typing.readthedocs.io/en/latest/source/stubs.html#version-and-platform-checks)
 #[violation]
 pub struct PatchVersionComparison;
 
@@ -104,6 +110,9 @@ impl Violation for PatchVersionComparison {
 /// if sys.version_info[0] == 3:
 ///     ...
 /// ```
+///
+/// ## References
+/// - [Typing stubs documentation: Version and Platform Checks](https://typing.readthedocs.io/en/latest/source/stubs.html#version-and-platform-checks)
 #[violation]
 pub struct WrongTupleLengthVersionComparison {
     expected_length: usize,
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unsupported_method_call_on_all.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unsupported_method_call_on_all.rs
index 5183fa580e725..3266dd4862d33 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unsupported_method_call_on_all.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unsupported_method_call_on_all.rs
@@ -17,14 +17,28 @@ use crate::checkers::ast::Checker;
 ///
 /// ## Example
 /// ```python
-/// __all__ = ["A"]
-/// __all__.append("B")
+/// import sys
+///
+/// __all__ = ["A", "B"]
+///
+/// if sys.version_info >= (3, 10):
+///     __all__.append("C")
+///
+/// if sys.version_info >= (3, 11):
+///     __all__.remove("B")
 /// ```
 ///
 /// Use instead:
 /// ```python
+/// import sys
+///
 /// __all__ = ["A"]
-/// __all__ += ["B"]
+///
+/// if sys.version_info < (3, 11):
+///     __all__ += ["B"]
+///
+/// if sys.version_info >= (3, 10):
+///     __all__ += ["C"]
 /// ```
 #[violation]
 pub struct UnsupportedMethodCallOnAll {
diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unused_private_type_definition.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unused_private_type_definition.rs
index cf4f7248b8aba..493bdbd7bf5dd 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unused_private_type_definition.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unused_private_type_definition.rs
@@ -45,7 +45,7 @@ impl Violation for UnusedPrivateTypeVar {
 ///
 /// ## Why is this bad?
 /// A private `typing.Protocol` that is defined but not used is likely a
-/// mistake, and should either be used, made public, or removed to avoid
+/// mistake. It should either be used, made public, or removed to avoid
 /// confusion.
 ///
 /// ## Example
@@ -83,11 +83,11 @@ impl Violation for UnusedPrivateProtocol {
 }
 
 /// ## What it does
-/// Checks for the presence of unused private `typing.TypeAlias` definitions.
+/// Checks for the presence of unused private type aliases.
 ///
 /// ## Why is this bad?
-/// A private `typing.TypeAlias` that is defined but not used is likely a
-/// mistake, and should either be used, made public, or removed to avoid
+/// A private type alias that is defined but not used is likely a
+/// mistake. It should either be used, made public, or removed to avoid
 /// confusion.
 ///
 /// ## Example
@@ -125,7 +125,7 @@ impl Violation for UnusedPrivateTypeAlias {
 ///
 /// ## Why is this bad?
 /// A private `typing.TypedDict` that is defined but not used is likely a
-/// mistake, and should either be used, made public, or removed to avoid
+/// mistake. It should either be used, made public, or removed to avoid
 /// confusion.
 ///
 /// ## Example