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

[flake8-pyi] Expand PYI018 to cover ParamSpecs and TypeVarTuples #9198

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

AlexWaygood
Copy link
Member

Summary

Part of #8771. flake8-pyi will emit a Y018 error for unused TypeVars, ParamSpecs or TypeVarTuples; Ruff currently only emits PYI018 for unused TypeVars.

This is my first "proper" Ruff PR -- let me know if there's a better way of doing this! Not sure if the repeated calls to match_typing_expr() are ideal.

Test Plan

I manually updated the fixtures to add some unused ParamSpecs and TypeVarTuples, and then updated the snapshots using cargo insta review. All tests then passed when run using cargo test.

@AlexWaygood AlexWaygood changed the title Pyi018 false negs Expand PYI018 to cover ParamSpecs and TypeVarTuples Dec 19, 2023
@AlexWaygood AlexWaygood changed the title Expand PYI018 to cover ParamSpecs and TypeVarTuples [flake8-pyi] Expand PYI018 to cover ParamSpecs and TypeVarTuples Dec 19, 2023
Copy link
Contributor

github-actions bot commented Dec 19, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Dec 19, 2023
_ => {
continue;
}
};
Copy link
Member

Choose a reason for hiding this comment

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

I might write this like:

let typevarlike_kind = if semantic.match_typing_expr(f, "TypeVar")  {
   "TypeVar"
} else if ...

But I don't know that what I'm proposing is actually better, I just hadn't seen this pattern before :)

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@charliermarsh charliermarsh enabled auto-merge (squash) December 20, 2023 03:04
Some("TypeVar")
} else if semantic.match_typing_call_path(&call_path, "ParamSpec") {
Some("ParamSpec")
} else if semantic.match_typing_call_path(&call_path, "TypeVarTuple") {
Copy link
Member

Choose a reason for hiding this comment

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

@AlexWaygood - I made one change here per your comment in the summary. Each call to semantic.match_typing_expr does a lookup internally to map the expression to a "call path", like ["typing", "TypeVar"]. However, we can just do that lookup once via semantic.resolve_call_path, then pass the call path to semantic.match_typing_call_path. (semantic.match_typing_expr is just a wrapper around this process, so if we want to do multiple checks, it's more efficient to do the lookup outside of the semantic.match_typing_expr.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh nice, thanks! Yep, that sounds like it's exactly what I was looking for :)

@charliermarsh charliermarsh merged commit bc0bf6f into astral-sh:main Dec 20, 2023
16 checks passed
@AlexWaygood AlexWaygood deleted the pyi018-false-negs branch December 20, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants