From c242104bafd4c63c0f2ed2e36651849378d7b267 Mon Sep 17 00:00:00 2001 From: Alexander Bock Date: Mon, 13 Feb 2023 22:16:23 +0100 Subject: [PATCH] Improve the reporting of specification errors --- apps/OpenSpace/main.cpp | 7 +----- .../openspace/documentation/documentation.h | 2 ++ modules/globebrowsing/src/layermanager.cpp | 8 +------ modules/statemachine/statemachinemodule.cpp | 2 +- src/documentation/documentation.cpp | 24 +++++++++++++++++++ src/engine/moduleengine.cpp | 7 +----- src/engine/openspaceengine.cpp | 7 +----- src/navigation/pathnavigator.cpp | 7 +----- src/navigation/pathnavigator_lua.inl | 2 +- src/scene/scene_lua.inl | 2 +- src/scripting/scriptengine.cpp | 7 +----- src/util/taskloader.cpp | 7 +----- 12 files changed, 36 insertions(+), 46 deletions(-) diff --git a/apps/OpenSpace/main.cpp b/apps/OpenSpace/main.cpp index d012932ca5..9c97586b33 100644 --- a/apps/OpenSpace/main.cpp +++ b/apps/OpenSpace/main.cpp @@ -1199,12 +1199,7 @@ int main(int argc, char* argv[]) { } catch (const documentation::SpecificationError& e) { LFATALC("main", "Loading of configuration file failed"); - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERRORC(o.offender, ghoul::to_string(o.reason)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNINGC(w.offender, ghoul::to_string(w.reason)); - } + logError(e); ghoul::deinitialize(); exit(EXIT_FAILURE); } diff --git a/include/openspace/documentation/documentation.h b/include/openspace/documentation/documentation.h index 65a7531cd0..a0164d6df4 100644 --- a/include/openspace/documentation/documentation.h +++ b/include/openspace/documentation/documentation.h @@ -121,6 +121,8 @@ struct SpecificationError : public ghoul::RuntimeError { TestResult result; }; +void logError(const SpecificationError& error, std::string component = ""); + struct Verifier; /** diff --git a/modules/globebrowsing/src/layermanager.cpp b/modules/globebrowsing/src/layermanager.cpp index ad6e514f8c..03b7cb5c43 100644 --- a/modules/globebrowsing/src/layermanager.cpp +++ b/modules/globebrowsing/src/layermanager.cpp @@ -99,13 +99,7 @@ Layer* LayerManager::addLayer(layers::Group::ID id, const ghoul::Dictionary& lay return _layerGroups[static_cast(id)]->addLayer(layerDict); } catch (const documentation::SpecificationError& e) { - LERRORC(e.component, e.message); - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERRORC(o.offender, ghoul::to_string(o.reason)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNINGC(w.offender, ghoul::to_string(w.reason)); - } + logError(e); return nullptr; } catch (const ghoul::RuntimeError& e) { diff --git a/modules/statemachine/statemachinemodule.cpp b/modules/statemachine/statemachinemodule.cpp index dfda04c8ee..3a4a15b807 100644 --- a/modules/statemachine/statemachinemodule.cpp +++ b/modules/statemachine/statemachinemodule.cpp @@ -69,8 +69,8 @@ void StateMachineModule::initializeStateMachine(const ghoul::Dictionary& states, )); } catch (const documentation::SpecificationError& e) { - LERROR(ghoul::to_string(e.result)); LERROR(fmt::format("Error loading state machine: {}", e.what())); + logError(e); } } diff --git a/src/documentation/documentation.cpp b/src/documentation/documentation.cpp index 6048deed89..f5ba040530 100644 --- a/src/documentation/documentation.cpp +++ b/src/documentation/documentation.cpp @@ -147,6 +147,30 @@ SpecificationError::SpecificationError(TestResult res, std::string comp) ghoul_assert(!result.success, "Result's success must be false"); } +void logError(const SpecificationError& error, std::string component) { + if (error.result.success) { + // Nothing to print if we succeeded + return; + } + + if (component.empty()) { + LERRORC(error.component, error.message); + } + else { + LERRORC(fmt::format("{}: {}", component, error.component), error.message); + + } + for (const documentation::TestResult::Offense& o : error.result.offenses) { + LERRORC( + o.offender, + fmt::format("{}: {}", ghoul::to_string(o.reason), o.explanation) + ); + } + for (const documentation::TestResult::Warning& w : error.result.warnings) { + LWARNINGC(w.offender, ghoul::to_string(w.reason)); + } +} + DocumentationEntry::DocumentationEntry(std::string k, std::shared_ptr v, Optional opt, std::string doc) : key(std::move(k)) diff --git a/src/engine/moduleengine.cpp b/src/engine/moduleengine.cpp index 7e559ec041..334d2a6241 100644 --- a/src/engine/moduleengine.cpp +++ b/src/engine/moduleengine.cpp @@ -82,12 +82,7 @@ void ModuleEngine::initialize( m->initialize(configuration); } catch (const documentation::SpecificationError& e) { - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERRORC(e.component, o.offender + ": " + ghoul::to_string(o.reason)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNINGC(e.component, w.offender + ": " + ghoul::to_string(w.reason)); - } + logError(e); throw; } diff --git a/src/engine/openspaceengine.cpp b/src/engine/openspaceengine.cpp index e8a3eee090..85d834ce86 100644 --- a/src/engine/openspaceengine.cpp +++ b/src/engine/openspaceengine.cpp @@ -265,12 +265,7 @@ void OpenSpaceEngine::initialize() { } catch (const documentation::SpecificationError& e) { LERROR("Failed loading of log"); - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERRORC(o.offender, ghoul::to_string(o.reason)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNINGC(w.offender, ghoul::to_string(w.reason)); - } + logError(e); throw; } } diff --git a/src/navigation/pathnavigator.cpp b/src/navigation/pathnavigator.cpp index 0753b6fb47..b1ffcb6f01 100644 --- a/src/navigation/pathnavigator.cpp +++ b/src/navigation/pathnavigator.cpp @@ -274,12 +274,7 @@ void PathNavigator::createPath(const ghoul::Dictionary& dictionary) { } catch (const documentation::SpecificationError& e) { LERROR("Could not create camera path"); - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERRORC(o.offender, ghoul::to_string(o.reason)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNINGC(w.offender, ghoul::to_string(w.reason)); - } + logError(e); } catch (const PathCurve::TooShortPathError&) { // Do nothing diff --git a/src/navigation/pathnavigator_lua.inl b/src/navigation/pathnavigator_lua.inl index e58816f612..26d3e79bce 100644 --- a/src/navigation/pathnavigator_lua.inl +++ b/src/navigation/pathnavigator_lua.inl @@ -172,7 +172,7 @@ namespace { ); } catch (const documentation::SpecificationError& e) { - LERRORC("flyToNavigationState", ghoul::to_string(e.result)); + logError(e, "flyToNavigationState"); throw ghoul::lua::LuaError(fmt::format("Unable to create a path: {}", e.what())); } diff --git a/src/scene/scene_lua.inl b/src/scene/scene_lua.inl index 0b3701aeb9..fe0a178d2c 100644 --- a/src/scene/scene_lua.inl +++ b/src/scene/scene_lua.inl @@ -603,7 +603,7 @@ namespace { node.hasValue("Identifier") ? node.value("Identifier") : "Scene"; - LERRORC(cat, ghoul::to_string(e.result)); + logError(e, cat); throw ghoul::lua::LuaError( fmt::format("Error loading scene graph node: {}", e.what()) diff --git a/src/scripting/scriptengine.cpp b/src/scripting/scriptengine.cpp index 06a0d99b48..65f9299315 100644 --- a/src/scripting/scriptengine.cpp +++ b/src/scripting/scriptengine.cpp @@ -445,12 +445,7 @@ void ScriptEngine::addLibraryFunctions(lua_State* state, LuaLibrary& library, library.documentations.push_back(std::move(func)); } catch (const documentation::SpecificationError& e) { - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERRORC(o.offender, ghoul::to_string(o.reason)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNINGC(w.offender, ghoul::to_string(w.reason)); - } + logError(e); } lua_pop(state, 1); } diff --git a/src/util/taskloader.cpp b/src/util/taskloader.cpp index 651a541d30..39d38e51e6 100644 --- a/src/util/taskloader.cpp +++ b/src/util/taskloader.cpp @@ -92,12 +92,7 @@ std::vector> TaskLoader::tasksFromFile(const std::string& } catch (const documentation::SpecificationError& e) { LERROR(fmt::format("Could not load tasks file {}. {}", absTasksFile, e.what())); - for (const documentation::TestResult::Offense& o : e.result.offenses) { - LERROR(ghoul::to_string(o)); - } - for (const documentation::TestResult::Warning& w : e.result.warnings) { - LWARNING(ghoul::to_string(w)); - } + logError(e); return std::vector>(); }