Refactor so that log warning only appears once per operation.

Author: thomashampson

Framework/API/inc/MantidAPI/AnalysisDataService.h

@@ -82,8 +82,10 @@ class MANTID_API_DLL AnalysisDataServiceImpl final : public Kernel::DataService<
   //@}
 
 public:
-  /// Is the given name a valid name for an object in the ADS
-  const std::string isValid(const std::string &name) const;
+  /// Validate the name and issue a warning in release mode, only return error string in debug mode.
+  const std::string isValid(const std::string &name, const bool printWarning = false) const;
+  //// Check whether the name is a valid python variable name and return an error message if not.
+  const std::string validateName(const std::string &name) const;
   /// Overridden add member to attach the name to the workspace when a
   /// workspace object is added to the service
   void add(const std::string &name, const std::shared_ptr<API::Workspace> &workspace) override;

Framework/API/src/AnalysisDataService.cpp

@@ -32,28 +32,26 @@ std::shared_ptr<const WorkspaceGroup> AnalysisDataServiceImpl::GroupUpdatedNotif
 // Public methods
 //-------------------------------------------------------------------------
 /**
- * Is the given name a valid name for an object in the ADS?
+ * Check whether the name is valid for the ADS. In release mode, issue a warning but do not return the error.
+ * In debug mode, return the error string.
  * @param name A string containing a possible name for an object in the ADS
  * @return An empty string if the name is valid or an error message stating the
  * problem if the name is unacceptable.
  */
-const std::string AnalysisDataServiceImpl::isValid(const std::string &name) const {
-  std::regex validName("^[a-zA-Z_][\\w]*");
-
-  if (!std::regex_match(name, validName)) {
-    const std::string error =
-        "Invalid object name '" + name +
-        "'. Names must start with a letter or underscore and contain only alpha-numeric characters and underscores.";
-
-    // Log a warning message to let user know we will stop allowing invalid variable names in the future.
-    const std::string warningMessage =
-        error + "\n"
-                "Currently this is only a warning, but in a future version of Mantid this will raise an exception and "
-                "the operation will fail.\n"
-                "Please update your scripts to use valid Python identifiers for workspace names.";
-    Kernel::Logger log("AnalaysisDataService");
-    log.warning(warningMessage);
-
+const std::string AnalysisDataServiceImpl::isValid(const std::string &name, const bool printWarning) const {
+  const auto error = validateName(name);
+  if (!error.empty()) {
+    if (printWarning) {
+      // Log a warning message to let user know we will stop allowing invalid variable names in the future.
+      const std::string warningMessage =
+          error +
+          "\n"
+          "Currently this is only a warning, but in a future version of Mantid this will raise an exception and "
+          "the operation will fail.\n"
+          "Please update your scripts to use valid Python identifiers for workspace names.";
+      Kernel::Logger log("AnalaysisDataService");
+      log.warning(warningMessage);
+    }
     // Fail in debug mode if name is not valid.
 #ifndef NDEBUG
     return error;
@@ -63,6 +61,23 @@ const std::string AnalysisDataServiceImpl::isValid(const std::string &name) cons
   return "";
 }
 
+/**
+ * Check whether the name is a valid python variable name, i.e. starts with underscore or letter, and contains
+ * only "word" characters (numbers, letters, underscores).
+ * @param name A string containing the name to validate
+ * @return An empty string if the name is valid or an error message describing the problem with the name.
+ */
+const std::string AnalysisDataServiceImpl::validateName(const std::string &name) const {
+  std::regex validName("^[a-zA-Z_][\\w]*");
+  std::string error = "";
+  if (!std::regex_match(name, validName)) {
+    error =
+        "Invalid object name '" + name +
+        "'. Names must start with a letter or underscore and contain only alpha-numeric characters and underscores.";
+  }
+  return error;
+}
+
 /**
  * Overwridden add member to attach the name to the workspace when a workspace
  * object is added to the service
@@ -404,9 +419,8 @@ AnalysisDataServiceImpl::AnalysisDataServiceImpl()
  * valid then it will check it's container for the same name that is passed if
  * true throws std::runtime_error else nothing.
  */
-
 void AnalysisDataServiceImpl::verifyName(const std::string &name, const std::shared_ptr<API::WorkspaceGroup> &group) {
-  const std::string error = isValid(name);
+  const std::string error = isValid(name, true);
   if (!error.empty()) {
     throw std::invalid_argument(error);
   }

Framework/API/test/AnalysisDataServiceTest.h

@@ -42,36 +42,36 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite {
 
   void setUp() override { ads.clear(); }
 
-  void testIsValidReturnsEmptyStringForValidPythonNames() {
-    TS_ASSERT_EQUALS(ads.isValid("a"), "");
-    TS_ASSERT_EQUALS(ads.isValid("Z"), "");
-    TS_ASSERT_EQUALS(ads.isValid("camelCase"), "");
-    TS_ASSERT_EQUALS(ads.isValid("PascalCase"), "");
-    TS_ASSERT_EQUALS(ads.isValid("has_Underscore"), "");
-    TS_ASSERT_EQUALS(ads.isValid("_starts_with_underscore"), "");
-    TS_ASSERT_EQUALS(ads.isValid("ends_with_underscore_"), "");
-    TS_ASSERT_EQUALS(ads.isValid("__l_o_t_s__o_f__u_n_d_e_r_s_c_o_r_e_s__"), "");
-    TS_ASSERT_EQUALS(ads.isValid("alllowercase"), "");
-    TS_ASSERT_EQUALS(ads.isValid("ALLUPPERCASE"), "");
-    TS_ASSERT_EQUALS(ads.isValid("Numb3rs"), "");
-    TS_ASSERT_EQUALS(ads.isValid("_m0r3_numb3r5"), "");
-    TS_ASSERT_EQUALS(ads.isValid("_"), "");
-    TS_ASSERT_EQUALS(ads.isValid("___"), "");
-  }
-
-  void testIsValidReturnsErrorStringForNamesContainingIllegalCharacters() {
+  void testValidateNameReturnsEmptyStringForValidPythonNames() {
+    TS_ASSERT_EQUALS(ads.validateName("a"), "");
+    TS_ASSERT_EQUALS(ads.validateName("Z"), "");
+    TS_ASSERT_EQUALS(ads.validateName("camelCase"), "");
+    TS_ASSERT_EQUALS(ads.validateName("PascalCase"), "");
+    TS_ASSERT_EQUALS(ads.validateName("has_Underscore"), "");
+    TS_ASSERT_EQUALS(ads.validateName("_starts_with_underscore"), "");
+    TS_ASSERT_EQUALS(ads.validateName("ends_with_underscore_"), "");
+    TS_ASSERT_EQUALS(ads.validateName("__l_o_t_s__o_f__u_n_d_e_r_s_c_o_r_e_s__"), "");
+    TS_ASSERT_EQUALS(ads.validateName("alllowercase"), "");
+    TS_ASSERT_EQUALS(ads.validateName("ALLUPPERCASE"), "");
+    TS_ASSERT_EQUALS(ads.validateName("Numb3rs"), "");
+    TS_ASSERT_EQUALS(ads.validateName("_m0r3_numb3r5"), "");
+    TS_ASSERT_EQUALS(ads.validateName("_"), "");
+    TS_ASSERT_EQUALS(ads.validateName("___"), "");
+  }
+
+  void testValidateNameReturnsErrorStringForNamesContainingIllegalCharacters() {
     const std::string illegalChars = " +-/*\\%<>&|^~=!@()[]{},:.`$'\"?";
     for (const char &illegalChar : illegalChars) {
       const std::string illegalName = std::string("variable_name") + illegalChar;
       std::string expectedError =
           "Invalid object name '" + illegalName +
           "'. Names must start with a letter or underscore and contain only alpha-numeric characters and underscores.";
-      TS_ASSERT_EQUALS(ads.isValid(illegalName), expectedError);
+      TS_ASSERT_EQUALS(ads.validateName(illegalName), expectedError);
     }
   }
 
-  void testIsValidReturnsErrorStringForNamesStartingWithNumbers() {
-    TS_ASSERT_EQUALS(ads.isValid("7dodgy_name"),
+  void testValidateNameReturnsErrorStringForNamesStartingWithNumbers() {
+    TS_ASSERT_EQUALS(ads.validateName("7dodgy_name"),
                      "Invalid object name '7dodgy_name'. Names must start with a letter or underscore and contain only "
                      "alpha-numeric characters and underscores.");
   }
@@ -151,13 +151,9 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite {
     TS_ASSERT_THROWS_NOTHING(ads.remove(name));
   }
 
-  void test_Add_With_Name_Containing_Illegal_Characters_Throws_Invalid_Argument() {
-    this->doAddingOnInvalidNameTests(false /*Don't use replace*/);
-  }
+  void testAddWithInvalidName() { this->doAddingOnInvalidNameTests(false /*Don't use replace*/); }
 
-  void test_AddOrReplace_With_Name_Containing_Illegal_Characters_Throws_Invalid_Argument() {
-    this->doAddingOnInvalidNameTests(true /*Use replace*/);
-  }
+  void testAddOrReplaceWithInvalidName() { this->doAddingOnInvalidNameTests(true /*Use replace*/); }
 
   void test_AddOrReplace_Does_Not_Throw_When_Adding_Object_That_Has_A_Name_That_Already_Exists() {
     const std::string name("MySpaceAddOrReplace");
@@ -542,18 +538,29 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite {
     for (const char &illegalChar : illegalChars) {
       // Build illegal name
       std::string name = std::string("ws") + illegalChar + "name";
-      // Add it
+
+#ifndef NDEBUG
+      // In debug mode, illegal workspace names throw exceptions.
       std::string errorMsg =
           std::string("Expected ADS to throw with illegal character ") + illegalChar + " in workspace name.";
       if (replace) {
         TSM_ASSERT_THROWS(errorMsg, addToADS(name), const std::invalid_argument &);
       } else {
         TSM_ASSERT_THROWS(errorMsg, addOrReplaceToADS(name), const std::invalid_argument &);
       }
-      bool stored = ads.doesExist(name);
-      TS_ASSERT_EQUALS(stored, false);
-      if (stored)
-        ads.remove(name); // Clear up if the test fails so that it dones't impact on others.
+      TS_ASSERT(!ads.doesExist(name));
+#else
+      // In release mode, a warning is printed but no exception is thrown.
+      std::string errorMsg =
+          std::string("Expected ADS not to throw with illegal character ") + illegalChar + " in workspace name.";
+      if (replace) {
+        TSM_ASSERT_THROWS_NOTHING(errorMsg, addToADS(name));
+      } else {
+        TSM_ASSERT_THROWS_NOTHING(errorMsg, addOrReplaceToADS(name));
+      }
+      TS_ASSERT(ads.doesExist(name));
+#endif
+      ads.remove(name); // Clear up if the test fails so that it dones't impact on others.
     }
   }