Remove the default value for '--data-template' and stop auto-deleting the data directory#1210
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1210 +/- ##
==========================================
- Coverage 55.59% 55.44% -0.16%
==========================================
Files 142 140 -2
Lines 20346 20270 -76
Branches 4974 4958 -16
==========================================
- Hits 11312 11238 -74
- Misses 5963 5973 +10
+ Partials 3071 3059 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/main/core/manager.c
Outdated
| if (g_file_test(manager->dataPath, G_FILE_TEST_EXISTS)) { | ||
| gboolean success = utility_removeAll(manager->dataPath); | ||
| utility_assert(success); | ||
| critical("data directory already exists"); |
There was a problem hiding this comment.
Would be helpful to include the path in this message
| if (g_file_test(templateDataPath, G_FILE_TEST_EXISTS)) { | ||
| gboolean success = utility_copyAll(templateDataPath, manager->dataPath); | ||
| utility_assert(success); | ||
| const gchar* templateDataPath = options_getDataTemplatePath(options); |
There was a problem hiding this comment.
Might be worth breaking this out into a helper function. If so you can use early returns to unnest some of this logic. e.g.
if (!g_file_test(absTemplatePath, G_FILE_TEST_EXISTS) {
critical("...")
return
}
...
(Or if we upgrade these to error we can unnest either way)
There was a problem hiding this comment.
I made these call error(), and now they're not longer nested.
| controller->bootstrapEndTime, managerSeed, element->preloadPath.string->str, | ||
| element->environment.isSet ? element->environment.string->str : NULL); | ||
|
|
||
| if (controller->manager == NULL) { |
There was a problem hiding this comment.
It's a bit surprising to have to check a new function for a NULL return. Since this is the only call site it's probably not that big of a deal in practice, but for an error that's ultimately going to result in exiting the simulation we might as well just error at that point rather than bubbling errors back up.
If we do want to make such a method fallible, we should probably call it something else; maybe tryNew
There was a problem hiding this comment.
I made all of the new code call error() instead of returning NULL, but I left the NULL check here since manager_new() can already return NULL from the existing code (since only a single manager can exist at a time), so I think it's still good to have this check.
a44acb4 to
53e3e42
Compare
Closes #1191.