Refactor settings #1200
Refactor settings #1200
Conversation
mu/settings.py
Outdated
| # handlers so they are saved when the Mu process exits | ||
| # | ||
| user = _UserSettings() | ||
| user.load(os.path.join(config.DATA_DIR, "settings.json")) |
There was a problem hiding this comment.
Since we can fail due to the open as well as the load, needing some guards, maybe add a topic about failure modes and strategies for missing, invalid, unreadable etc. files? Amnesia or reset modes could be triggered from these errors.
There was a problem hiding this comment.
Heh :) Great minds and all that. I already have some uncommitted code which does some useful guarding both around the load and the save elements. Definitely a good idea to note the direction of travel in the design docs, though.
There was a problem hiding this comment.
I've added some notes here:
Implement the existing save_session functionality via the settings module
|
This pull request introduces 1 alert when merging 9c68fda into 4241844 - view on LGTM.com new alerts:
|
| * A failure to read the JSON from a settings file is more complicated. For | ||
| pragmatic purposes, the intention is here is: log a warning; quarantine the | ||
| file; and carry on as though it had been found empty. That way the editor | ||
| continues to work, albeit in "reset" mode, and the failing file is available | ||
| for debugging. |
There was a problem hiding this comment.
Although not something to be done by this module itself, I think we should show an error to the user.
The most likely situation of a malformed JSON file is the user trying to change a setting, and in that case loading the defaults without immediate visible warning can be confusing and/or misleading.
There was a problem hiding this comment.
@carlosperate agreed, but that's part of a wider piece: how to keep the user informed during startup. I have a notion about essentially extending the current "Splash" frame to have a status line with a slot which startup processes can signal, indicating progress or errors / failures etc.
Of course, we might have a failure even to start up a UI at all. But I regard that as a slightly different problem
Keep track of changes to only user updates are written back to settings files Refactor to allow better separation of serialisation (JSON? TOML? etc.) and writing file Switch to using a settings file for the virtual environment config
|
This pull request introduces 5 alerts when merging d8f43a0 into 4241844 - view on LGTM.com new alerts:
|
|
This pull request introduces 5 alerts when merging 8df8dca into 4241844 - view on LGTM.com new alerts:
|
(Mostly consists in making sure the settings autosave functionality doesn't kick in)
…compatible_configs.html#flake8 to align Flake8's line length to Black's
…ic init function. This is both to assist in testing but also because having module-level registration seems like a recipe of unpredictability Allow the Settings save method to override the filename; again, mostly for ease of testing Switch to picking up baseline packages from the venv settings file, and skip now-redundant tests + Black
…ill already warn but continue if the file to be loaded doesn't exist
Update tests for mode base
|
@carlosperate one of the functions I relocated as part of the work on settings was, in effect, your commit: Do you know / can you outline the need here? I mean: the code's there and it works, so at one level it doesn't matter. But neither @ntoll nor I could remember the use case behind it. |
Allow for env vars to be expanded for string values + Tidy
It was added when the app was packaged as a single executable. In that case this was useful to be able to add a settings.json file in the same folder as mu.exe and it would pick up the settings from that file, so users could configure some of the "hidden" features. I would argue that a feature like this could still be useful, but not necessary. If we were to update this it should perhaps look for the settings file in the At the time this was introduced it had certainly helped people easily change settings without having to figure out where the local "app settings" directory was located, which is usually a multi-step process. |
Thanks, @carlosperate; that's just the kind of information I was looking for. For now, the functionality remains there unchanged, but we might revisit in any later reconsideration of where settings file are searched. |
…are replaced before other tests run
(not picked up in existing tests because we haven't got a good way to test the slots mechanism)
Addressing #1184
Organise access to settings / session data so it happens via global singletons and saves automatically
(Looking forward) probably need to allow override of filenames and "amnesia mode"