Extracted json saving and loading #10216
Conversation
homeassistant/util/__init__.py
Outdated
| """ | ||
| try: | ||
| with open(filename, 'w', encoding='utf-8') as fdesc: | ||
| fdesc.write(json.dumps(config, sort_keys=True, indent=4)) |
There was a problem hiding this comment.
We should json dump to a string before opening the file. That way, when dumping fails we don't truncate the file.
homeassistant/util/__init__.py
Outdated
| try: | ||
| with open(filename, 'w', encoding='utf-8') as fdesc: | ||
| fdesc.write(json.dumps(config, sort_keys=True, indent=4)) | ||
| return True |
homeassistant/util/__init__.py
Outdated
| @@ -116,6 +121,38 @@ def get_random_string(length=10): | |||
| return ''.join(generator.choice(source_chars) for _ in range(length)) | |||
|
|
|||
|
|
|||
| def load_json(filename: str) -> Union[List, Dict]: | |||
There was a problem hiding this comment.
Let's move these files to their own package util.json
homeassistant/util/__init__.py
Outdated
| except FileNotFoundError: | ||
| _LOGGER.debug('JSON file not found: %s', filename) | ||
| except ValueError: | ||
| _LOGGER.warning('Could not parse JSON content: %s', filename) |
There was a problem hiding this comment.
This error is pretty bad, we should probably just blow up?
There was a problem hiding this comment.
You are right. I didn't want to change the behavior much from what was currently in place/use.
I'll raise a HomeAsssistantError, analogous to load_yaml, instead.
There was a problem hiding this comment.
This means that we do no longer support empty files?
homeassistant/util/__init__.py
Outdated
| except ValueError: | ||
| _LOGGER.warning('Could not parse JSON content: %s', filename) | ||
| except OSError as error: | ||
| _LOGGER.warning('JSON file reading failed: %s (%s)', |
There was a problem hiding this comment.
This error is pretty bad, we should probably just blow up?
homeassistant/util/__init__.py
Outdated
| with open(filename, 'w', encoding='utf-8') as fdesc: | ||
| fdesc.write(json.dumps(config, sort_keys=True, indent=4)) | ||
| return True | ||
| except OSError as error: |
There was a problem hiding this comment.
Same here, should we blow up to make sure that both user and code knows it did not manage to save anything.
|
This is a great initiative! Especially |
homeassistant/util/json.py
Outdated
| """ | ||
| try: | ||
| with open(filename, 'w', encoding='utf-8') as fdesc: | ||
| data = json.dumps(config, sort_keys=True, indent=4) |
There was a problem hiding this comment.
We should do this before opening the file, otherwise the file has already been truncated.
ghost
added
Description:
Many components use json files to store custom configuration data.
All implements their own very basic code for loading and saving dicts as JSON files (
with open(filename) as f: json.loads(f)). Some do error checking (checking if file exists or not), some do not. Some catch relevant exceptions, some do not.It is obvious that in most places, the exact same logic is taking place, and in many places the same code is duplicated (a 20-line function
config_from_file(filename, config=None)seems to be copied around a lot).This PR provides basic functionality for loading and saving dicts as JSON (
load_json,save_json, analogous withload_yaml) in core.Benefits: Less code duplication, hopefully somewhat better error handling, better reporting in the logs, and easier testing/mocking.
The PR only contain changes in one component (
media_player/plex.py) to useload_jsonandsave_json, but at least the places mentioned in #10269 could possibly also benefit from this change.Related issue (if applicable): fixes #
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.github.io#<home-assistant.github.io PR number goes here>
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass