Y068: Don't use @override #528
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
* Supports typing_extension * Error message is now attached to decorator, not function
This comment has been minimized.
This comment has been minimized.
| def check_for_override(self, node: ast.FunctionDef | ast.AsyncFunctionDef) -> None: | ||
| for deco in node.decorator_list: | ||
| if _is_override(deco): | ||
| self.error(deco, errors.Y068) | ||
| return |
There was a problem hiding this comment.
or we could just ban it from being imported by adding a branch to _check_import_or_attribute, rather than emitting a separate diagnostic on each usage in every stub file?
There was a problem hiding this comment.
I thought about it, but I like this direct approach a bit more: While it may spam a bit in case someone uses it, it's more "direct". "Here's your error", instead of "You're importing something here that's going to be a problem later." It still allows overriding the warning on a case-by-cases basis (although I'm not sure why you would need that). No strong opinion, though.
There was a problem hiding this comment.
I feel like emitting the diagnostic at the location of the import (or attribute access) is more in keeping with our other rules such as Y024 and Y057... but I also don't have a strong opinion :-)
There was a problem hiding this comment.
Ok, then let's laziness win and not change it?
This comment has been minimized.
This comment has been minimized.
ERRORCODES.md
Outdated
| | <a id="Y066" href="#Y066">Y066</a> | When using if/else with `sys.version_info`, put the code for new Python versions first. | Style | ||
| | <a id="Y067" href="#Y067">Y067</a> | Don't use `Incomplete | None = None` in | ||
| argument annotations. Instead, just use `=None`. | Style | ||
| | <a id="Y068" href="#Y068">Y068</a> | Don't use `@override` in stub files. | Correctness |
There was a problem hiding this comment.
Could we maybe give an explanation here, such as "Other tools such as stubtest are better placed to catch issues where stubs deviate from the runtime code, so @override just adds visual noise to the stub".
I'd also possibly classify this one under the "Understanding stubs" category? It seems like it's a tool that only really makes sense for runtime code, so it arguably demonstrates a misunderstanding of how stubs work if you use it in a stub
There was a problem hiding this comment.
Should I add the explanation to the ERRORCODES file only or to the actual error message?
I prefer the former (because of visual noise, I know, I know ...), but again no strong opinions.
There was a problem hiding this comment.
I also prefer the former. It's not too hard to look up the error in ERRORCODES.md, and we're able to link directly to the docs for the specific error code. If we had a nice way of rendering multiline diagnostics, maybe things would be different, but we don't (and I have no appetite for implementing that 😆)
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Alex Waygood <[email protected]>
|
This change has no effect on typeshed. 🤖🎉 |
2 participants
Closes: #527