-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Additional type hints for config module, part 2. #11480
Changes from 2 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 |
---|---|---|
@@ -0,0 +1 @@ | ||
Add missing type hints to `synapse.config` module. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1257,7 +1257,9 @@ def add_arguments(parser: argparse.ArgumentParser) -> None: | |
help="Turn on the twisted telnet manhole service on the given port.", | ||
) | ||
|
||
def read_gc_intervals(self, durations) -> Optional[Tuple[float, float, float]]: | ||
def read_gc_intervals( | ||
self, durations: Optional[List[Any]] | ||
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. Are we using 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. I don't think we know that this is a 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. Ah yes indeed. Though following this logic, how do we know that it's a list? afaict we feed it the config directly without checking the type. 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. Good point! I'll change it to |
||
) -> Optional[Tuple[float, float, float]]: | ||
"""Reads the three durations for the GC min interval option, returning seconds.""" | ||
if durations is None: | ||
return 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.
I wasn't really thrilled with this
Any
here, we do assert that the types are correct below (via JSON schema), but I don't think that all quite works with mypy...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.
Looks like this type could be
List[JsonDict]
?Especially since that's what
_perspectives_to_key_servers
is typed to return, and it returns data in the same format askey_servers
.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.
All we know is that it is a list, we then verify that the list is the proper form below, using JSON schema. The input is from YAML so could really be anything.
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.
Oh, hmmm I see what you're saying it is either a hard-coded value of the result of
_perspectives_to_key_servers
, so maybe we can do better here, yes!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.
Ah, but we can't since we extend it with
_perspectives_to_key_servers
, so the original gunk could still just be weird values. We really have no idea what it is.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.
Ah yeah good point.