-
-
Notifications
You must be signed in to change notification settings - Fork 38.1k
Make typing checks more strict #14429
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
Changes from all commits
41ef7e7
0d472d7
0c3a591
14afc72
2ffe76b
821fcce
d55cec7
d80f613
35b1c3f
55a69fa
61c5886
0bd1259
193d12f
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 |
|---|---|---|
|
|
@@ -8,8 +8,6 @@ | |
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| _UNDEFINED = object() | ||
|
|
||
|
|
||
| class SerializationError(HomeAssistantError): | ||
| """Error serializing the data to JSON.""" | ||
|
|
@@ -19,7 +17,7 @@ class WriteError(HomeAssistantError): | |
| """Error writing the data.""" | ||
|
|
||
|
|
||
| def load_json(filename: str, default: Union[List, Dict] = _UNDEFINED) \ | ||
| def load_json(filename: str, default: Union[List, Dict, None] = None) \ | ||
| -> Union[List, Dict]: | ||
| """Load JSON data from a file and return as dict or list. | ||
|
|
||
|
|
@@ -37,7 +35,7 @@ def load_json(filename: str, default: Union[List, Dict] = _UNDEFINED) \ | |
| except OSError as error: | ||
| _LOGGER.exception('JSON file reading failed: %s', filename) | ||
| raise HomeAssistantError(error) | ||
| return {} if default is _UNDEFINED else default | ||
| return {} if default is None else default | ||
|
Member
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. This not the same. By removing the sentinel value, it is no longer possible to have
Contributor
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. There were no uses of
Member
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. There was no use yet, as the storage helper is very new. I don't think that we should change this.
Contributor
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. The storage helper was the lone use. I meant that no one else used By using sentinel value the function becomes not type-safe, as it accepts Another option is to have
Member
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 feel too strongly I guess. Any place that was using
Member
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. Actually, you could also make the sentinal value an empty dict, that way you can keep it all as dict. Comparison is still done with
Contributor
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. It will generate a warning that the default value is mutable. Lets keep it |
||
|
|
||
|
|
||
| def save_json(filename: str, data: Union[List, Dict]): | ||
|
|
@@ -46,9 +44,9 @@ def save_json(filename: str, data: Union[List, Dict]): | |
| Returns True on success. | ||
| """ | ||
| try: | ||
| data = json.dumps(data, sort_keys=True, indent=4) | ||
| json_data = json.dumps(data, sort_keys=True, indent=4) | ||
| with open(filename, 'w', encoding='utf-8') as fdesc: | ||
| fdesc.write(data) | ||
| fdesc.write(json_data) | ||
| except TypeError as error: | ||
| _LOGGER.exception('Failed to serialize to JSON: %s', | ||
| filename) | ||
|
|
||
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.
Is this the same? Is a non timezone aware date that is being localized treated as having been UTC time? Or would it treat it as local time?
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.
Here is the implementation of UTC.localize