Separate data from executable #90
Conversation
|
Also related #29 |
|
Hi. Thanks for your work! |
|
I actually just implemented global settings and going to add an installation path option (doing this right in this PR) |
Yeah we're not in a hurry and I'm still adding stuff to this PR so that's fine |
There was a problem hiding this comment.
Whew, finally got some time to look at this (but not super thoroughly). Sorry for disappearing on you.
I ran into issues trying to launch it, because of nulls, etc. (see my comments). Once you take care of them, I'll take a more thorough look at it and maybe throw in some suggestions (or add them myself).
Gotta run now. Hopefully it won't be a month until next time :)
| catapult_dir = _determine_catapult_dir() | ||
| settings_dir = _determine_settings_dir() | ||
|
|
||
| Settings.load_settings(settings_dir) |
There was a problem hiding this comment.
This is not needed, it is called automatically when Settings.read is called for the first time.
| settings_dir = _determine_settings_dir() | ||
|
|
||
| Settings.load_settings(settings_dir) | ||
| var inst_dir_setting = Settings.read("installation_dir") |
There was a problem hiding this comment.
There needs to be a hard-coded default value for installation_dir, just like for all the other settings. Returning null here creates all sorts of problems. For example, calling replace() below on what is potentially a null. Consider an empty string as the default.
🙃 Hi! Sorry for not answering for so long, I had a busy December in my university. I'll be able to continue working on this and look into your suggestions in about a week when my exams are over. And Happy New Year! |
Related to #89