-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add flake-pyi PYI033 "Do not use type comments in stubs" #3302
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# OK | ||
x = 1 # type: int | ||
|
||
|
||
# OK | ||
def reverse(x): # type:(str) -> str | ||
... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# ERROR: PYI033 Don't use type comments in stub file | ||
x = 1 # type: int | ||
|
||
|
||
# ERROR: PYI033 Don't use type comments in stub file | ||
def reverse(x): # type:(str) -> str | ||
... |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
use rustpython_parser::lexer::LexResult; | ||
use rustpython_parser::Tok; | ||
|
||
use ruff_macros::{define_violation, derive_message_formats}; | ||
|
||
use crate::registry::Diagnostic; | ||
use crate::violation::Violation; | ||
use crate::Range; | ||
|
||
define_violation!( | ||
/// ## What it does | ||
/// Do not use type comments (e.g. `x = 1 # type: int`) in stubs, even if | ||
/// the stub supports Python 2. Always use annotations instead (e.g. | ||
/// `x: int = 1`). | ||
/// | ||
/// ## Why is this bad? | ||
/// You should use type annotation directly in pyi files. | ||
/// | ||
/// ## Example | ||
/// ```python | ||
/// x = 1 # type: int | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// x: int = 1 | ||
/// ``` | ||
pub struct TypeCommentInStub; | ||
); | ||
impl Violation for TypeCommentInStub { | ||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
format!("Don't use type comments in stub file") | ||
} | ||
} | ||
|
||
/// PYI033 | ||
pub fn type_comment_in_stub(tokens: &[LexResult]) -> Vec<Diagnostic> { | ||
let mut diagnostics = vec![]; | ||
|
||
for token in tokens.iter().flatten() { | ||
if let (location, Tok::Comment(comment), end_location) = token { | ||
// I couldn't find any PEP on the exact syntax (the closest being | ||
// https://peps.python.org/pep-0484/#type-comments), but every case I saw used | ||
// `# type:` verbatim so this seems to be the right thing to pick | ||
if comment.starts_with("# type:") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this also be true for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. currently yes; are there any cases where you need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish ignore comments wouldn't be needed in stubs but they are used. Here is an example from typeshed https://github.com/python/typeshed/blob/45f0a5e7e47a23d6be67cfce429c4f2c0de62a29/stdlib/builtins.pyi#L565 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for the example! i'll except those then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All our flake8-pyi tests for this rule are in this file FYI, if that helps :) https://github.com/PyCQA/flake8-pyi/blob/main/tests/type_comments.pyi There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth looking at the In particular, they have this regex, which omits There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oops thanks i somehow used them for Y006 but then forgot that those also exist for Y033. thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fwiw i had to do two regexes since rust regex doesn't support look around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @AlexWaygood for the helpful comment :) |
||
diagnostics.push(Diagnostic::new( | ||
TypeCommentInStub, | ||
Range { | ||
location: *location, | ||
end_location: *end_location, | ||
}, | ||
)); | ||
} | ||
} | ||
} | ||
|
||
diagnostics | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
source: crates/ruff/src/rules/flake8_pyi/mod.rs | ||
expression: diagnostics | ||
--- | ||
[] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
--- | ||
source: crates/ruff/src/rules/flake8_pyi/mod.rs | ||
expression: diagnostics | ||
--- | ||
- kind: | ||
TypeCommentInStub: ~ | ||
location: | ||
row: 2 | ||
column: 6 | ||
end_location: | ||
row: 2 | ||
column: 17 | ||
fix: ~ | ||
parent: ~ | ||
- kind: | ||
TypeCommentInStub: ~ | ||
location: | ||
row: 6 | ||
column: 17 | ||
end_location: | ||
row: 6 | ||
column: 36 | ||
fix: ~ | ||
parent: ~ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,6 +206,8 @@ Options: | |
Run in watch mode by re-running whenever files change | ||
--fix-only | ||
Fix any fixable lint violations, but don't report on leftover violations. Implies `--fix` | ||
--ignore-noqa | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure how this got in here, should this be here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh strange. Builds should fail if |
||
Ignore any `# noqa` comments | ||
--format <FORMAT> | ||
Output serialization format for violations [env: RUFF_FORMAT=] [possible values: text, json, junit, grouped, github, gitlab, pylint] | ||
--target-version <TARGET_VERSION> | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this paragraph would make sense in the
## Why is it bad?
section, since## What it does?
tends to be a description of what it's checking rather than alternatives etc. So e.g.:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've also added the
# type: ignore
exception