Fix/collection defaults (#33)

* (wip) fix overriding vector defaults * revert default changes and fix map * fix printing issue with newer gtest
MIT-SPARK · Sep 17, 2024 · f569658 · f569658
1 parent 5553441
commit f569658
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 9 deletions.
diff --git a/config_utilities/include/config_utilities/internal/visitor_impl.hpp b/config_utilities/include/config_utilities/internal/visitor_impl.hpp
@@ -231,10 +231,15 @@ void Visitor::visitField(std::vector<ConfigT>& config, const std::string& field_
   }
 
   if (visitor.mode == Visitor::Mode::kSet) {
+    const auto array_ns = visitor.name_space.empty() ? field_name : visitor.name_space + "/" + field_name;
+    const auto subnode = lookupNamespace(visitor.data.data, array_ns);
+    if (!subnode) {
+      return;  // don't override the field if not present
+    }
+
     // When setting the values first allocate the correct amount of configs.
     config.clear();
-    const auto array_ns = visitor.name_space.empty() ? field_name : visitor.name_space + "/" + field_name;
-    const std::vector<YAML::Node> nodes = getNodeArray(lookupNamespace(visitor.data.data, array_ns));
+    const std::vector<YAML::Node> nodes = getNodeArray(subnode);
     size_t index = 0;
     for (const auto& node : nodes) {
       ConfigT& sub_config = config.emplace_back();
@@ -274,11 +279,8 @@ void Visitor::visitField(std::map<K, ConfigT>& config, const std::string& field_
     return;
   }
 
-  OrderedMap<K, ConfigT> intermediate;
-  // copy current config state if doing something else other than settings the config
-  if (visitor.mode != Visitor::Mode::kSet) {
-    intermediate.insert(intermediate.begin(), config.begin(), config.end());
-  }
+  // copy current config state
+  OrderedMap<K, ConfigT> intermediate(config.begin(), config.end());
 
   // make use of more general named parsing
   visitField<K, ConfigT>(intermediate, field_name, unit);
@@ -301,10 +303,15 @@ void Visitor::visitField(OrderedMap<K, ConfigT>& config, const std::string& fiel
   }
 
   if (visitor.mode == Visitor::Mode::kSet) {
+    const auto map_ns = visitor.name_space.empty() ? field_name : visitor.name_space + "/" + field_name;
+    const auto subnode = lookupNamespace(visitor.data.data, map_ns);
+    if (!subnode) {
+      return;  // don't override the field if not present
+    }
+
     // When setting the values first allocate the correct amount of configs.
     config.clear();
-    const auto map_ns = visitor.name_space.empty() ? field_name : visitor.name_space + "/" + field_name;
-    const auto nodes = getNodeMap(lookupNamespace(visitor.data.data, map_ns));
+    const auto nodes = getNodeMap(subnode);
     for (auto&& [key, node] : nodes) {
       auto& entry = config.emplace_back();
       entry.first = key.template as<K>();

diff --git a/config_utilities/test/tests/config_arrays.cpp b/config_utilities/test/tests/config_arrays.cpp
@@ -105,6 +105,8 @@ class AddString : public ProcessorBase {
       config::RegistrationWithConfig<ProcessorBase, AddString, AddString::Config>("AddString");
 };
 
+bool operator==(const AddString::Config& lhs, const AddString::Config& rhs) { return lhs.s == rhs.s; }
+
 void declare_config(AddString::Config& config) {
   name("AddString");
   field(config.s, "s");
@@ -128,6 +130,21 @@ void declare_config(ConfigWithNestedArray& config) {
   field(config.nested, "nested");
 }
 
+struct ConfigWithDefaultArray {
+  std::vector<AddString::Config> configs{{"world!"}, {"hello"}};
+};
+
+void declare_config(ConfigWithDefaultArray& config) {
+  name("ConfigWithDefaultArray");
+  field(config.configs, "configs");
+}
+
+bool operator==(const ConfigWithDefaultArray& lhs, const ConfigWithDefaultArray& rhs) {
+  return lhs.configs == rhs.configs;
+}
+
+void PrintTo(const ConfigWithDefaultArray& config, std::ostream* os) { *os << toString(config); }
+
 TEST(ConfigArrays, FromYamlSeq) {
   const std::string yaml_seq = R"(
 - s: "a"
@@ -407,4 +424,21 @@ config_array[2] [ArrConfig]:
   EXPECT_EQ(formatted, expected);
 }
 
+TEST(ConfigArrays, ArrayWithDefaults) {
+  {  // make sure not specifying anything results in default
+    const auto node = YAML::Load("");
+    auto config = fromYaml<ConfigWithDefaultArray>(node);
+    EXPECT_EQ(config.configs.size(), 2);
+    EXPECT_EQ(config, ConfigWithDefaultArray());
+  }
+
+  {  // specifying empty list clears default
+    const auto node = YAML::Load("configs: []");
+    auto config = fromYaml<ConfigWithDefaultArray>(node);
+    EXPECT_EQ(config.configs.size(), 0);
+    ConfigWithDefaultArray expected{{}};
+    EXPECT_EQ(config, expected);
+  }
+}
+
 }  // namespace config::test
diff --git a/config_utilities/test/tests/config_maps.cpp b/config_utilities/test/tests/config_maps.cpp
@@ -99,6 +99,19 @@ void declare_config(ConfigWithNestedMaps& config) {
   field(config.nested, "nested");
 }
 
+struct ConfigWithDefaultMap {
+  std::map<std::string, MapConfig> configs{{"name1", {"world!", 1}}, {"name2", {"hello", 3}}};
+};
+
+void declare_config(ConfigWithDefaultMap& config) {
+  name("ConfigWithDefaultMap");
+  field(config.configs, "configs");
+}
+
+bool operator==(const ConfigWithDefaultMap& lhs, const ConfigWithDefaultMap& rhs) { return lhs.configs == rhs.configs; }
+
+void PrintTo(const ConfigWithDefaultMap& config, std::ostream* os) { *os << toString(config); }
+
 TEST(ConfigMaps, FromYamlMap) {
   const std::string yaml_map = R"(
 x:
@@ -278,4 +291,21 @@ config_map[4] [MapConfig]:
   EXPECT_EQ(formatted, expected);
 }
 
+TEST(ConfigMaps, MapWithDefault) {
+  {  // make sure not specifying anything results in default
+    const auto node = YAML::Load("");
+    auto config = fromYaml<ConfigWithDefaultMap>(node);
+    EXPECT_EQ(config.configs.size(), 2);
+    EXPECT_EQ(config, ConfigWithDefaultMap());
+  }
+
+  {  // specifying empty list clears default
+    const auto node = YAML::Load("configs: {}");
+    auto config = fromYaml<ConfigWithDefaultMap>(node);
+    EXPECT_EQ(config.configs.size(), 0);
+    ConfigWithDefaultMap expected{{}};
+    EXPECT_EQ(config, expected);
+  }
+}
+
 }  // namespace config::test