Refactor: open json files by JSONHandler#28
Refactor: open json files by JSONHandler#28acemon33 wants to merge 4 commits intoPherakki:developfrom
Conversation
…t "" to MainWindow "", which already has a layout'
There was a problem hiding this comment.
Overall this looks really good. I appreciate the effort made to fix the PEP8 violations for linespacing.
I think looking at this there are two things we should consider adding beyond the comments I've made to make our lives easier:
- A utility that formats an error message for an SDMM config JSON,
def sdmm_config_json_error_message(file):
return translate("Utilities", "SDMM config file '{file}' is not a valid JSON file").format(file=file)- Many of the error messages would be made much more useful if they included the mod name in the error message. Instead of passing around the LooseMod data structure everywhere to do this, I'd suggest we just use a utility to read from the METADATA.json whenever we hit a critical error like this and use that to get the mod name. It's a little bit of code duplication we can clean up at a later date (or if you're particularly motivated, you could find a way to abstract out common code with the metadata parser routines - which we really should do at some point to keep tech debt low). It doesn't matter that this is expensive because we should be hitting errors very infrequently, and the additional diagnostic information vastly outweighs what will be a totally unnoticeable performance hit.
def get_diagnostic_mod_name(mod_directory):
local_mod_directory = os.path.split(mod_directory)[1]
metadata_path = os.path.join(mod_directory, "METADATA.json")
error_mod_name = "<COULD NOT READ MOD NAME>"
unknown_mod_name = "<NAMELESS MOD>"
if os.path.exists(metadata_path):
try:
with JSONDecoder(metadata_path, "") as metadata_data:
mod_name = metadata_data.get("Name", error_mod_name)
except:
mod_name = error_mod_name
else:
mod_name = unknown_mod_name
return f"{mod_name} (mods/{local_mod_directory})"
def diagnostic_mod_name_getter_factory(formattable_string):
def wrapped_func(mod_directory):
return formattable_string.format(get_diagnostic_mod_name(mod_directory))
return wrapped_funcThis can then be used like
# File-level scope, near the top of the file
ALIASES_JSON_ERROR_MESSAGE_GENERATOR= diagnostic_mod_name_getter_factory(translate(..., "ALIASES.json for mod '{}' is not a valid JSON file"))
# File contents...
# The function using the JSON
with JSONHandler(filepath, lambda: ALIASES_JSON_ERROR_MESSAGE_GENERATOR(mod_directory)):
...We pass in a function as the message instead of a plain string so we can compute the 'expensive' string later. To make this work we'll need to extend the JSONHandler again to replace the except statement with
except json.decoder.JSONDecodeError as e:
if callable(self.message):
msg = self.message()
else:
msg = self.message
raise json.decoder.JSONDecodeError(f'{msg}: {str(e)}') from ePutting the initial closure generation near the top of the file saves us re-doing that work every time we hit the JSONHandler, which will save us a bit of perf (although I would bet this is essentially free compared to the cost of the rest of the mod installation).
| with zipfile.ZipFile(path, 'r') as F: | ||
| with F.open("METADATA.json", 'r') as fJ: | ||
| self.init_metadata(fJ) | ||
| self.init_metadata(json.load(fJ)) |
There was a problem hiding this comment.
This is a good change, but please also
- Implement this change on plugins/modformats/NestedZipMod.py
- Refer to my comment on the JSONHandler for how to also use in this context
| class JSONHandler: | ||
| def __init__(self, filename, message, **decode_kwargs): | ||
| self.filename = filename | ||
| self.message = message | ||
| self.decode_kwargs = decode_kwargs | ||
|
|
||
| def __enter__(self): | ||
| if not os.path.exists(self.filename): | ||
| raise FileNotFoundError(f'JSON file {self.filename} does not exist') | ||
| try: | ||
| with open(self.filename, 'r', encoding="utf-8") as stream: | ||
| return json.load(stream, **self.decode_kwargs) | ||
| except json.decoder.JSONDecodeError as e: | ||
| raise json.decoder.JSONDecodeError(f'{self.message}: {str(e)}') from e | ||
|
|
||
| def __exit__(self, exc_type, exc_value, exc_traceback): | ||
| pass |
There was a problem hiding this comment.
It looks like we need to extend the interface to be able to deal with already-existing streams. I'd propose we extend the interface as such:
class JSONHandler:
def __init__(self, filename, message, **decode_kwargs):
self.filename = filename
self.message = message
self.decode_kwargs = decode_kwargs
@staticmethod
def decode(stream, message, **decode_kwargs):
try:
return json.load(stream, **self.decode_kwargs)
except json.decoder.JSONDecodeError as e:
raise json.decoder.JSONDecodeError(f'{self.message}: {str(e)}') from e
def __enter__(self):
if not os.path.exists(self.filename):
raise FileNotFoundError(f'JSON file {self.filename} does not exist')
with open(self.filename, 'r', encoding="utf-8") as stream:
return self.decode(stream, self.message, **decode_kwargs)
def __exit__(self, exc_type, exc_value, exc_traceback):
passIt is then possible to use the JSON handler as
with open(...) as stream:
json_data = JSONHandler.decode(stream, "...")| def read_config(self): | ||
| with open(os.path.join(self.paths.config_loc, "config.json"), 'r') as F: | ||
| config_data = json.load(F) | ||
| with JSONHandler(os.path.join(self.paths.config_loc, "config.json"), "Error reading 'config.json'") as config_data: |
There was a problem hiding this comment.
Perhaps extend the message to "SDMM config file 'config.json' is not a valid JSON file" to make it clear to the user that the problematic file isn't inside a mod.
| def init_from_script(cls, filepath, log): | ||
| with open(filepath, 'r') as F: | ||
| cymis = json.load(F) | ||
| with JSONHandler(filepath, f"Error Reading '{filepath}'") as stream: |
There was a problem hiding this comment.
We shouldn't call this a stream since it's a dict and not a filestream; maybe change the variable name to 'json_data'.
| def init_from_script(cls, filepath, log): | ||
| with open(filepath, 'r') as F: | ||
| cymis = json.load(F) | ||
| with JSONHandler(filepath, f"Error Reading '{filepath}'") as stream: |
There was a problem hiding this comment.
I'd make the error message f"CYMIS script at {filepath}' is not a valid JSON file"
| with JSONHandler(os.path.join(self.paths.softcodes_loc, f"Error reading '{main_filename}'")) as stream: | ||
| dct = stream |
There was a problem hiding this comment.
stream->softcode_defs_data, f"Softcode Definitions file '{main_filename}' is not a valid JSON file"
| except json.decoder.JSONDecodeError as e: | ||
| print('error', e) |
There was a problem hiding this comment.
We shouldn't catch the error since we want it to be passed up to the log with the extra information it provides
| with JSONHandler(cache_loc, f"Error reading '{main_filename}'") as data: | ||
| dct["codes"] = data |
There was a problem hiding this comment.
f"Softcode definitions file '{main_filename}' is not a valid JSON file"
| with JSONHandler(self.ops.paths.localisations_names_loc, f"Error reading '{self.ops.paths.localisations_names_loc}'") as stream: | ||
| names = stream |
There was a problem hiding this comment.
f"SDMM config file {os.path.split(self.ops.paths.localisations_names_loc)[1]}' is not a valid JSON file"
| with JSONHandler(filepath, f"Error reading '{file}'") as stream: | ||
| style_def = stream |
There was a problem hiding this comment.
f"SDMM theme file '{file}' is not a valid JSON file"
Also fix some minor issues