Improve type hints #1666
Improve type hints #1666
Conversation
| @@ -52,7 +53,7 @@ def alert_should_be_present( | |||
| self, | |||
| text: str = "", | |||
| action: str = ACCEPT, | |||
| timeout: Union[float, str, None] = None, | |||
| timeout: Optional[timedelta] = None, | |||
There was a problem hiding this comment.
Optional[] is unnecessary when there's None as a default value. Even MyPy doesn't need it unless you run it with --no-implicit-optional:
https://mypy.readthedocs.io/en/stable/command_line.html?highlight=optional#cmdoption-mypy-no-implicit-optional
There was a problem hiding this comment.
I think Optional is best practice if None is used. Types should not contain None as part of the type and if they do, then that should explicitly be stated.
See for example https://en.wikipedia.org/wiki/Void_safety for more details on this subject.
There was a problem hiding this comment.
And the default is also changing in mypy python/mypy#9091
| @@ -76,7 +78,7 @@ def alert_should_be_present( | |||
|
|
|||
| @keyword | |||
| def alert_should_not_be_present( | |||
| self, action: str = ACCEPT, timeout: Union[float, str, None] = 0 | |||
| self, action: str = ACCEPT, timeout: Union[timedelta] = 0 | |||
There was a problem hiding this comment.
This is wrong because an argument with type timedelta cannot really have an integer as a default. You probably also meant Optional[timedelta] not Union[timedelta] (but I'd leave Optional[] away as well).
There was a problem hiding this comment.
Hmmm, what would be suitable default? The easiest would be to change it to None but that would be somewhat backwards incompatible (Because it would default to library default timeout.) Would:
def alert_should_not_be_present(self, action: str = ACCEPT, timeout = timedelta(seconds=0);
be better?
No description provided.