Add flake-pyi PYI033 "Do not use type comments in stubs" #3302

konstin · 2023-03-02T12:18:21Z

I think this is also a case where people like want this for .py files, too, at least on 3.7+ (https://docs.python.org/3/whatsnew/3.7.html#pep-563-postponed-evaluation-of-annotations)

On that note, does none of the tools upgrade all type comments to type annotations automatically? I looked in pyupgrade and googled and was surprised i couldn't find anything

I think this is also a case where people like want this for .py files, too, at least on 3.7+ (https://docs.python.org/3/whatsnew/3.7.html#pep-563-postponed-evaluation-of-annotations) On that note, does none of the tools upgrade all type comments to type annotations automatically? I looked in pyupgrade and googled but couldn't find anything

konstin · 2023-03-02T14:40:12Z

docs/configuration.md

@@ -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


not sure how this got in here, should this be here?

Oh strange. Builds should fail if cargo dev generate-all produces any changed files. Lemme look into it.

twoertwein · 2023-03-02T14:53:04Z

crates/ruff/src/rules/flake8_pyi/rules/type_comment_in_stub.rs

+            // 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:") {


Would this also be true for # type: ignore?

currently yes; are there any cases where you need # type: ignore in a .pyi file?

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

thanks for the example! i'll except those then

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

It might be worth looking at the flake8-pyi source and seeing how they handle this?

In particular, they have this regex, which omits ignore comments:

https://github.com/PyCQA/flake8-pyi/blob/66f28a4407af2156f95d370941fae4dd98280740/pyi.py#L1877

oops thanks i somehow used them for Y006 but then forgot that those also exist for Y033.

thanks!

fwiw i had to do two regexes since rust regex doesn't support look around

Thanks @AlexWaygood for the helpful comment :)

charliermarsh · 2023-03-02T15:51:01Z

On that note, does none of the tools upgrade all type comments to type annotations automatically? I looked in pyupgrade and googled and was surprised i couldn't find anything

There's a tool called com2ann which seems popular for this use-case. It's come up in Ruff before, though I've tried to cut scope at "tools that support one-time 2-to-3 conversion" :)

JonathanPlasse · 2023-03-02T16:49:22Z

Maybe com2ann should be mentioned in the rule explanation.

charliermarsh

LGTM! One suggestion on the docstring.

charliermarsh · 2023-03-02T17:55:01Z

crates/ruff/src/rules/flake8_pyi/rules/type_comment_in_stub.rs

+    /// ## 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`).


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.:

/// ## What it does /// Checks for the use of type comments (e.g., `x = 1 # type: int`) in stub /// files. /// /// ## Why is this bad? /// Stub (`.pyi`) files should always use type annotations directly, rather /// than type comments, even if they're intended to support Python 2, since /// stub files are not executed at runtime. /// /// ## Example /// ```python /// x = 1 # type: int /// ``` /// /// Use instead: /// ```python /// x: int = 1 /// ```

i've also added the # type: ignore exception

Co-authored-by: Charlie Marsh <[email protected]>

konstin requested a review from charliermarsh March 2, 2023 12:18

konstin force-pushed the flake8-pyi-Y033-type-comments-in-stub-files branch from ea05f84 to df50951 Compare March 2, 2023 14:09

konstin force-pushed the flake8-pyi-Y033-type-comments-in-stub-files branch from df50951 to 5e3481c Compare March 2, 2023 14:33

konstin commented Mar 2, 2023

View reviewed changes

twoertwein reviewed Mar 2, 2023

View reviewed changes

Use flake8-pyi tests and ignore # type: ignore

e44968c

charliermarsh approved these changes Mar 2, 2023

View reviewed changes

Improve TypeCommentInStub doc string

03665b2

Co-authored-by: Charlie Marsh <[email protected]>

charliermarsh merged commit 3bcffb5 into astral-sh:main Mar 3, 2023

charliermarsh added the rule Implementing or modifying a lint rule label Mar 3, 2023

konstin deleted the flake8-pyi-Y033-type-comments-in-stub-files branch March 3, 2023 15:50

JonathanPlasse mentioned this pull request Mar 4, 2023

Implement pygrep-hooks #980

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flake-pyi PYI033 "Do not use type comments in stubs" #3302

Add flake-pyi PYI033 "Do not use type comments in stubs" #3302

konstin commented Mar 2, 2023 •

edited

Loading

konstin Mar 2, 2023

charliermarsh Mar 2, 2023

twoertwein Mar 2, 2023

konstin Mar 2, 2023

twoertwein Mar 2, 2023

konstin Mar 2, 2023

AlexWaygood Mar 2, 2023

charliermarsh Mar 2, 2023

konstin Mar 2, 2023

konstin Mar 2, 2023

charliermarsh Mar 2, 2023

charliermarsh commented Mar 2, 2023

JonathanPlasse commented Mar 2, 2023

charliermarsh left a comment

charliermarsh Mar 2, 2023

konstin Mar 3, 2023

konstin Mar 3, 2023

Add flake-pyi PYI033 "Do not use type comments in stubs" #3302

Add flake-pyi PYI033 "Do not use type comments in stubs" #3302

Conversation

konstin commented Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh commented Mar 2, 2023

JonathanPlasse commented Mar 2, 2023

charliermarsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstin commented Mar 2, 2023 •

edited

Loading