Merge pull request #37873 from mantidproject/ewm6894-fix-ConfigService-remove

Expose `ConfigService::configureLogging()` to API, fix handling of removed properties not also in file

Author: KedoKudo

Framework/Kernel/inc/MantidKernel/ConfigService.h

@@ -206,7 +206,9 @@ class MANTID_KERNEL_DLL ConfigServiceImpl final {
   /// Sets the log level priority for all log channels
   void setLogLevel(int logLevel, bool quiet = false);
   /// Sets the log level priority for all log channels
-  void setLogLevel(std::string logLevel, bool quiet = false);
+  void setLogLevel(std::string const &logLevel, bool quiet = false);
+  // return the string name for the log level
+  std::string getLogLevel();
 
   /// Look for an instrument
   const InstrumentInfo &getInstrument(const std::string &instrumentName = "") const;

Framework/Kernel/src/ConfigService.cpp

@@ -721,11 +721,16 @@ void ConfigServiceImpl::saveConfig(const std::string &filename) const {
   } // End while-loop
 
   // Any remaining keys within the changed key store weren't present in the
-  // current user properties so append them
+  // current user properties so append them IF they exist
   if (!m_changed_keys.empty()) {
     updated_file += "\n";
     auto key_end = m_changed_keys.end();
     for (auto key_itr = m_changed_keys.begin(); key_itr != key_end;) {
+      // if the key does not have a property, skip it
+      if (!hasProperty(*key_itr)) {
+        ++key_itr;
+        continue;
+      }
       updated_file += *key_itr + "=";
       std::string value = getString(*key_itr, false);
       Poco::replaceInPlace(value, "\\", "\\\\"); // replace single \ with double
@@ -1916,7 +1921,7 @@ void ConfigServiceImpl::setLogLevel(int logLevel, bool quiet) {
   }
 }
 
-void ConfigServiceImpl::setLogLevel(std::string logLevel, bool quiet) {
+void ConfigServiceImpl::setLogLevel(std::string const &logLevel, bool quiet) {
   Mantid::Kernel::Logger::setLevelForAll(logLevel);
   // update the internal value to keep strings in sync
   m_pConf->setString(LOG_LEVEL_KEY, g_log.getLevelName());
@@ -1926,6 +1931,8 @@ void ConfigServiceImpl::setLogLevel(std::string logLevel, bool quiet) {
   }
 }
 
+std::string ConfigServiceImpl::getLogLevel() { return g_log.getLevelName(); }
+
 /// \cond TEMPLATE
 template DLLExport boost::optional<double> ConfigServiceImpl::getValue(const std::string &);
 template DLLExport boost::optional<std::string> ConfigServiceImpl::getValue(const std::string &);

Framework/Kernel/test/ConfigServiceTest.h

@@ -110,6 +110,19 @@ class ConfigServiceTest : public CxxTest::TestSuite {
     TS_ASSERT_THROWS_NOTHING(ConfigService::Instance().setLogLevel(4));
   }
 
+  void testLogLevelSetGet() {
+    Logger log1("testLogLevelGetSet");
+
+    for (std::string x : Logger::PriorityNames) {
+      TS_ASSERT_THROWS_NOTHING(ConfigService::Instance().setLogLevel(x));
+      TS_ASSERT_EQUALS(log1.getLevelName(), x);
+      TS_ASSERT_EQUALS(ConfigService::Instance().getLogLevel(), x);
+    }
+
+    // return back to previous values
+    TS_ASSERT_THROWS_NOTHING(ConfigService::Instance().setLogLevel(4));
+  }
+
   void testDefaultFacility() {
     TS_ASSERT_THROWS_NOTHING(ConfigService::Instance().getFacility());
     //
@@ -410,6 +423,50 @@ class ConfigServiceTest : public CxxTest::TestSuite {
     }
   }
 
+  void testRemoveNotPopulated() {
+    /* If a property was not originally set in the properties file,
+     *  but is later removed, it will still appear in a list or properties
+     *  to update.  This test ensures the removed properties are not present.
+     */
+
+    // Backup current user settings
+    ConfigServiceImpl &settings = ConfigService::Instance();
+    const std::string userFileBackup = settings.getUserFilename() + ".unittest";
+    try {
+      Poco::File userFile(settings.getUserFilename());
+      userFile.moveTo(userFileBackup);
+    } catch (Poco::Exception &) {
+    }
+
+    // pick a property that is not in the config file
+    std::string garbage("garbage.truck");
+    std::string keeps("garbage.keep"); // control property
+    TS_ASSERT(!settings.hasProperty(garbage));
+    TS_ASSERT(!settings.hasProperty(keeps));
+
+    // add the property to the config
+    settings.setString(garbage, "yes");
+    settings.setString(keeps, "yes");
+    TS_ASSERT(settings.hasProperty(garbage));
+    TS_ASSERT(settings.hasProperty(keeps));
+
+    // now remove the property and save.
+    // ensure the removed property is not inside the config file
+    settings.remove(garbage);
+    settings.saveConfig(settings.getUserFilename());
+    const std::string contents = readFile(settings.getUserFilename());
+    TS_ASSERT(!contents.empty());
+    TS_ASSERT(contents.find(garbage) == std::string::npos);
+    TS_ASSERT(contents.find(keeps) != std::string::npos);
+
+    // restore the old file
+    try {
+      Poco::File backup(userFileBackup);
+      backup.moveTo(settings.getUserFilename());
+    } catch (Poco::Exception &) {
+    }
+  }
+
   void testSaveConfigWithPropertyRemoved() {
     const std::string filename("user.settings.testSaveConfigWithPropertyRemoved");
     Poco::File prop_file(filename);

Framework/PythonInterface/mantid/kernel/src/Exports/ConfigService.cpp

@@ -104,6 +104,8 @@ void export_ConfigService() {
            "definitions")
       .def("getFacilityNames", &ConfigServiceImpl::getFacilityNames, arg("self"), "Returns the default facility")
       .def("getFacilities", &ConfigServiceImpl::getFacilities, arg("self"), "Returns the default facility")
+      .def("configureLogging", &ConfigServiceImpl::configureLogging, arg("self"),
+           "Configure and start the logging framework")
       .def("remove", &ConfigServiceImpl::remove, (arg("self"), arg("rootName")), "Remove the indicated key.")
       .def("getFacility", (const FacilityInfo &(ConfigServiceImpl::*)() const) & ConfigServiceImpl::getFacility,
            arg("self"), return_value_policy<reference_existing_object>(), "Returns the default facility")
@@ -144,10 +146,12 @@ void export_ConfigService() {
       .def("saveConfig", &ConfigServiceImpl::saveConfig, (arg("self"), arg("filename")),
            "Saves the keys that have changed from their default to the given "
            "filename")
+      .def("getLogLevel", &ConfigServiceImpl::getLogLevel, arg("self"),
+           "Return the string value for the log representation")
       .def("setLogLevel", (void (ConfigServiceImpl::*)(int, bool)) & ConfigServiceImpl::setLogLevel,
            (arg("self"), arg("logLevel"), arg("quiet") = false),
            "Sets the log level priority for all the log channels, logLevel 1 = Fatal, 6 = information, 7 = Debug")
-      .def("setLogLevel", (void (ConfigServiceImpl::*)(std::string, bool)) & ConfigServiceImpl::setLogLevel,
+      .def("setLogLevel", (void (ConfigServiceImpl::*)(std::string const &, bool)) & ConfigServiceImpl::setLogLevel,
            (arg("self"), arg("logLevel"), arg("quiet") = false),
            "Sets the log level priority for all the log channels. Allowed values are fatal, critical, error, warning, "
            "notice, information, debug, and trace.")

Framework/PythonInterface/test/python/mantid/kernel/ConfigServiceTest.py

@@ -133,6 +133,12 @@ def test_setting_log_channel_levels(self):
         testhelpers.assertRaisesNothing(self, config.setLogLevel, 4, True)
         testhelpers.assertRaisesNothing(self, config.setLogLevel, "warning", True)
 
+    def test_log_level_get_set(self):
+        logLevels = ["fatal", "error", "warning", "information", "debug"]
+        for x in logLevels:
+            config.setLogLevel(x)
+            self.assertEqual(config.getLogLevel(), x)
+
     def test_properties_documented(self):
         # location of the rst file relative to this file this will break if either moves
         doc_filename = os.path.split(__file__)[0]

buildconfig/CMake/CppCheck_Suppressions.txt.in

@@ -91,7 +91,6 @@ returnByReference:${CMAKE_SOURCE_DIR}/Framework/Kernel/inc/MantidKernel/ConfigSe
 returnByReference:${CMAKE_SOURCE_DIR}/Framework/Kernel/inc/MantidKernel/ConfigService.h:197
 virtualCallInConstructor:${CMAKE_SOURCE_DIR}/Framework/Kernel/inc/MantidKernel/ThreadScheduler.h:174
 virtualCallInConstructor:${CMAKE_SOURCE_DIR}/Framework/Kernel/inc/MantidKernel/ThreadScheduler.h:278
-passedByValue:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/ConfigService.cpp:1919
 constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/TopicInfo.cpp:38
 returnStdMoveLocal:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/FilteredTimeSeriesProperty.cpp:408
 constVariablePointer:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/PropertyManagerProperty.cpp:149

docs/source/release/v6.11.0/Workbench/New_features/37873.rst

@@ -0,0 +1,3 @@
+- expose :class:`ConfigService::configureLogging() <mantid.kernel.ConfigServiceImpl.configureLogging>` to python API
+- expose :class:`ConfigService::getLogLevel() <mantid.kernel.ConfigServiceImpl.getLogLevel>` to python API
+- fix handling of removed propertyies in :class:`ConfigService::saveConfig() <mantid.kernel.ConfigServiceImpl.saveConfig>`