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.

Sign up for GitHub

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

[flake8-type-checking] Expands TC006 docs to better explain itself #14749

Merged
merged 4 commits into from
Dec 4, 2024
Merged

[flake8-type-checking] Expands TC006 docs to better explain itself #14749

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2a1fb13
[`flake8-type-checking`] Expands TC006 docs to better explain itself
Daverball Dec 3, 2024
03b135e
Updates docs according to latest discussion
Daverball Dec 4, 2024
0a1cfb6
Applies Alex's suggestion
Daverball Dec 4, 2024
c4b160a
Adds additional note about performance
Daverball Dec 4, 2024
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
5 changes: 4 additions & 1 deletion crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_cast_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ use crate::rules::flake8_type_checking::helpers::quote_type_expression;
///
/// ## Why is this bad?
/// `typing.cast()` does not do anything at runtime, so the time spent
/// on evaluating the type expression is wasted.
/// on evaluating the potentially complex type expression is wasted.
/// But it's also bad to be inconsistent, so this rule always quotes
/// the type expression even if its contribution to import/evaluation
/// time is negligible, as a matter of style.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From our conversation on the issue, my conclusion is that this rule should be thought of as primarily about enforcing stylistic consistency, since there will be other rules added in the future that will be better suited to deal with the performance aspects. I think the first sentence of these docs should reflect that, but the first sentence here still makes it sound like it's primarily focussed on performance :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to phrase this any differently, you're welcome to try.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this?

 /// ## What it does
-/// Checks for an unquoted type expression in `typing.cast()` calls.
+/// Checks for unquoted type expressions in `typing.cast()` calls.
 ///
 /// ## Why is this bad?
-/// `typing.cast()` does not do anything at runtime, so the time spent
-/// on evaluating the potentially complex type expression is wasted.
-/// But it's also bad to be inconsistent, so this rule always quotes
-/// the type expression even if its contribution to import/evaluation
-/// time is negligible, as a matter of style.
+/// This rule helps enforce a consistent style across your codebase.
+///
+/// It's often necessary to quote the first argument passed to `cast()`,
+/// as type expressions can involve forward references, or references
+/// to symbols which are only imported in `typing.TYPE_CHECKING` blocks.
+/// This can lead to a visual inconsistency across different `cast()` calls,
+/// where some type expressions are quoted but others are not. By enabling
+/// this rule, you ensure that all type expressions passed to `cast()` are
+/// quoted, enforcing stylistic consistency across all of your `cast()` calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems fine to me. Although I'd eventually like to cross-reference performance-focused alternatives like a quote-casts setting for TC001-003 and/or a new rule to quote type expressions that create typing._GenericAlias instances. So if we completely stop explicitly mentioning performance that might come a bit out of left-field.

At least as long as we make the quoting sub-expressions part of TC001-003 and this potentially new rule opt-in. If it's always on (which should technically always be safe) then I don't think that's as necessary. But given your readability pushback I assume there are some people which would prefer to never quote their casts, regardless of any potential benefits.

So in that world I would like to give people enough information in order to make an informed decision about whether TC006, enabling quote-casts or doing neither would make more sense for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a sentence to the end about performance?

 /// ## What it does
-/// Checks for an unquoted type expression in `typing.cast()` calls.
+/// Checks for unquoted type expressions in `typing.cast()` calls.
 ///
 /// ## Why is this bad?
-/// `typing.cast()` does not do anything at runtime, so the time spent
-/// on evaluating the potentially complex type expression is wasted.
-/// But it's also bad to be inconsistent, so this rule always quotes
-/// the type expression even if its contribution to import/evaluation
-/// time is negligible, as a matter of style.
+/// This rule helps enforce a consistent style across your codebase.
+///
+/// It's often necessary to quote the first argument passed to `cast()`,
+/// as type expressions can involve forward references, or references
+/// to symbols which are only imported in `typing.TYPE_CHECKING` blocks.
+/// This can lead to a visual inconsistency across different `cast()` calls,
+/// where some type expressions are quoted but others are not. By enabling
+/// this rule, you ensure that all type expressions passed to `cast()` are
+/// quoted, enforcing stylistic consistency across all of your `cast()` calls.
+///
+/// In some cases where `cast()` is used in a hot loop, this rule may also
+/// help avoid overhead from repeatedly evaluating complex type expressions at
+/// runtime.

You don't have to use my suggested rewrite exactly. But from the discussion on the issue, I do think the emphasis should be on style/consistency, rather than performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could lead into that with something like the following

/// Although quoting the type expression in `typing.cast` calls
/// will ensure that the runtime overhead is as small as possible
/// there are alternative rules and settings which will get you
/// most of the way there, while keeping quoted expressions to
/// a minimum.

So your text seems good enough for now.

///
/// ## Example
/// ```python
Expand Down
Loading