-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add help: subdiagnostics for several Ruff rules that can sometimes appear to disagree with ty
#22331
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 help: subdiagnostics for several Ruff rules that can sometimes appear to disagree with ty
#22331
Changes from all commits
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 |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| use ruff_macros::{ViolationMetadata, derive_message_formats}; | ||
| use ruff_python_ast as ast; | ||
| use ruff_python_ast::identifier::Identifier; | ||
| use ruff_python_semantic::analyze::function_type::is_subject_to_liskov_substitution_principle; | ||
| use ruff_python_semantic::analyze::{function_type, visibility}; | ||
|
|
||
| use crate::Violation; | ||
|
|
@@ -121,11 +122,24 @@ pub(crate) fn too_many_arguments(checker: &Checker, function_def: &ast::StmtFunc | |
| return; | ||
| } | ||
|
|
||
| checker.report_diagnostic( | ||
| let mut diagnostic = checker.report_diagnostic( | ||
| TooManyArguments { | ||
| c_args: num_arguments, | ||
| max_args: checker.settings().pylint.max_args, | ||
| }, | ||
| function_def.identifier(), | ||
| ); | ||
| if is_subject_to_liskov_substitution_principle( | ||
|
Contributor
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. Can we use the helper here? Oh, I see it's an FBT helper.
Member
Author
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. Right — I could put the helper somewhere more central, but wasn't totally sure where a good place would be :/ |
||
| &function_def.name, | ||
| &function_def.decorator_list, | ||
| semantic.current_scope(), | ||
| semantic, | ||
| &checker.settings().pep8_naming.classmethod_decorators, | ||
| &checker.settings().pep8_naming.staticmethod_decorators, | ||
| ) { | ||
| diagnostic.help( | ||
| "Consider adding `@typing.override` if changing the function signature \ | ||
| would violate the Liskov Substitution Principle", | ||
| ); | ||
| } | ||
| } | ||
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 kind of prefer this phrasing over the LSP version, but I don't feel strongly. I'm guessing we can't call the helper function here because the current scope isn't set?
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.
That wasn't really my rationale for doing something slightly different here. The reasons why I did something slightly different here is that this violation is telling you to rename the function rather than change its signature, and renaming the function isn't going to cause a Liskov violation. It's just not necessarily what you want to do if the function is deliberately overriding something from a subclass 😄