-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Stabilize generic-not-last-base-class (PYI059)
#18601
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
Conversation
Summary -- The tests were already in the right place, I just updated the documentation slightly to reflect the discussion [here](#18519 (comment)) and on Discord. Documentation: https://docs.astral.sh/ruff/rules/generic-not-last-base-class Tests: https://github.com/astral-sh/ruff/blob/brent/release-0.12.0/crates/ruff_linter/src/rules/flake8_pyi/mod.rs#L51-L52 Test Plan -- Existing tests
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PYI059 | 13 | 13 | 0 | 0 | 0 |
Linter (preview)
✅ ecosystem check detected no linter changes.
|
related: #18602 |
IMO I think that probably does block stabilisation, but I agree with you that for now we could just not offer a fix if there are any keyword or variadic arguments, stabilise it after making that change, and open a followup issue to add a fix for classes that have keyword arguments. Variadic arguments (things like |
| /// The rule is also applied to stub files, where it won't cause issues at runtime, | ||
| /// because type checkers may also behave unpredictably in this case. |
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 I'd probably say something like "type checkers may not be able to infer an accurate MRO for for the class, which could lead to unexpected or inaccurate results when they analyze your code". Linking to the glossary sounds like a great idea!
Co-authored-by: Alex Waygood <[email protected]>
crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs
Outdated
Show resolved
Hide resolved
…_base_class.rs Co-authored-by: Alex Waygood <[email protected]>
|
Closing this in light of the discussion on #18611 (need to move the fix to unsafe, updating the kwarg handling), but I'll copy the docs improvements over there. |
Co-authored-by: Alex Waygood <[email protected]>
Summary
The tests were already in the right place, I just updated the documentation slightly to reflect the discussion here and on Discord.
Documentation: https://docs.astral.sh/ruff/rules/generic-not-last-base-class
Tests: https://github.com/astral-sh/ruff/blob/brent/release-0.12.0/crates/ruff_linter/src/rules/flake8_pyi/mod.rs#L51-L52
I considered mentioning the MRO explicitly and linking to this but decided that might be a bit too technical for the rule docs.
Now I see that ty links to the glossary, which would be another option.
Test Plan
Existing tests