Catch and ignore errors writing JSON files.#3288
Conversation
There was a problem hiding this comment.
I would add a statement here that the program behavior will still be correct if any given call to write_cache is replaced with a no-op. That makes it easier to see the logic in the other comments. Edit: by "here" I meant near the definition of write_cache.
|
LGTM |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Looks good to me -- I only have some suggested refactorings. Feel free to merge when you are ready.
| data_mtime = os.path.getmtime(data_json_tmp) | ||
| os.replace(data_json_tmp, data_json) | ||
| manager.trace("Interface for {} has changed".format(id)) | ||
| try: |
There was a problem hiding this comment.
Suggested refactoring: Move the try statement to a separate utility function to make the flow of the enclosing function easier to follow. Currently the function is too long to fit entirely on my screen, which makes code reviews harder.
There was a problem hiding this comment.
It's unfortunate that the error handling (including comments) is longer than the main logic of the function, but I'm hesitant about breaking it up into smaller bits. Maybe it'll fit on your screen if I move the comments elsewhere (e.g. to the function's docstring)? Anyway, we can iterate on that after the merge.
| else: | ||
| json.dump(meta, f) | ||
| os.replace(meta_json_tmp, meta_json) | ||
| try: |
There was a problem hiding this comment.
Like above, maybe move this to a separate function?
3 participants
This is an alternative attempt at fixing issue #3215, given the
complexity of PR #3239.