-
Notifications
You must be signed in to change notification settings - Fork 22
🔧 mypy fixes #117
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
base: master
Are you sure you want to change the base?
🔧 mypy fixes #117
Conversation
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.
final_argument_whitespace = True | ||
|
||
def __init__(self, *args, **kwargs): | ||
def __init__(self, *args: object, **kwargs: object) -> None: |
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.
The right fix is to remove the whole constructor. That's the default of what Python would do anyway.
|
||
|
||
# Re-export type name for readability in this file | ||
MappingEntryType = MappingEntry |
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.
why not rename the source?
If you really want to do this, the right way is
from sphinxcontrib.test_reports.jsonparser import MappingEntry as MappingEntryType
""" | ||
|
||
def __init__(self, *args, **kwargs): | ||
def __init__(self, *args: object, **kwargs: object) -> None: |
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.
There is no point in writing object
all the time. It just makes the code less readable. Either you know what may come here / how it is used, so you can properly handle specific args or kwargs or you ignore the mypy error.
I think object is also wrong, because you can't do anything with it, e.g. it cannot be assigned to string so any type checker sees attributes such as .upper()
.
If you use object:
Type checker says “I don’t know what these args are, and you can’t use them for anything specific.”
That’s correct but restrictive.
If you use Any:
Type checker says “these args could be anything, and I won’t stop you from doing stuff with them.”
That’s looser but matches the idea of blindly forwarding *args, **kwargs.
I would either spell out all params or do this
def __init__(self, *args: Any, **kwargs: Any) -> None:
super().__init__(*args, **kwargs)
"""Collect any extra options and their values that were specified in the directive""" | ||
tr_extra_options = getattr(self.app.config, "tr_extra_options", []) | ||
self.extra_options = {} | ||
tr_extra_options = cast(Optional[List[str]], getattr(self.app.config, "tr_extra_options", None)) |
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.
should use lower case variants of List
and Dict
, just list
and dict
. No import needed for that.
self.extra_options = extra | ||
|
||
def load_test_file(self): | ||
def load_test_file(self) -> Optional[List[Dict[str, object]]]: |
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.
Use
def load_test_file(self) -> Optional[List[Dict[str, object]]]: | |
def load_test_file(self) -> list[dict[str, Any]]] | None: |
And ideally find out what Any actually is if you can :)
elif value in ("FALSE", "NO", "0"): | ||
self.collapse = False | ||
else: | ||
raise Exception("collapse attribute must be true or false") |
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.
the message is incorrect. it can also be no/yes/0/1
No description provided.