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.

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

fix: fix PYI024 lint rule violation #205

Closed
wants to merge 1 commit into from

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Aug 26, 2023

% ruff rule PYI024

collections-named-tuple (PYI024)

Derived from the flake8-pyi linter.

What it does

Checks for uses of collections.namedtuple in stub files.

Why is this bad?

typing.NamedTuple is the "typed version" of collections.namedtuple.

The class generated by subclassing typing.NamedTuple is equivalent to
collections.namedtuple, with the exception that typing.NamedTuple
includes an __annotations__ attribute, which allows type checkers to
infer the types of the fields.

Example

from collections import namedtuple


person = namedtuple("Person", ["name", "age"])

Use instead:

from typing import NamedTuple


class Person(NamedTuple):
    name: str
    age: int

@cclauss
Copy link
Contributor

cclauss commented Aug 26, 2023

This rule only has power when we are type-checking with a tool like mypy.

Neither gyp-next nor node-gyp are currently running mypy but that would be a nice addition.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

cygwin = int(rule.get("msvs_cygwin_shell",
                            self.spec.get("msvs_cygwin_shell", 1))) != 0

Would seem to create a bool.

@@ -935,7 +935,9 @@ def BuildCygwinBashCommandLine(self, args, path_to_base):
)
return cmd

RuleShellFlags = collections.namedtuple("RuleShellFlags", ["cygwin", "quote"])
class RuleShellFlags(typing.NamedTuple):
cygwin: int
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that mypy would agree with this change but without running mypy in our GitHub Actions, I would be reluctant to merge this PR.

Suggested change
cygwin: int
cygwin: bool

@rzhao271
Copy link
Contributor Author

Closing this PR
I tried running mypy locally and there's a lot of objects that need manual annotations

@rzhao271 rzhao271 closed this Aug 26, 2023
@rzhao271 rzhao271 deleted the rzhao271/lint-pyi024 branch August 26, 2023 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants