addressing code review. added optional string struct, function for co…

…mbining set, error handling for argument parsing

xml_converter/integration_tests/run_tests.py

@@ -20,7 +20,7 @@ def run_xml_converter(
     input_proto: Optional[List[str]] = None,
     output_proto: Optional[List[str]] = None,
     split_output_proto: Optional[str] = None,
-    additional_arguments: Optional[List[str]] = None,
+    allow_duplicates: Optional[bool] = None,
 ) -> Tuple[str, str, int]:
 
     # Build the command to execute the C++ program with the desired function and arguments
@@ -36,8 +36,8 @@ def run_xml_converter(
         cmd += ["--output-guildpoint-path"] + output_proto
     if split_output_proto:
         cmd += ["--output-split-guildpoint-path"] + [split_output_proto]
-    if additional_arguments:
-        cmd += additional_arguments
+    if allow_duplicates:
+        cmd += ["--allow-duplicates"]
 
     # Run the C++ program and capture its output
     result = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True)
@@ -177,7 +177,7 @@ def main() -> bool:
             input_proto=testcase.proto_input_paths,
             output_xml=[xml_output_dir_path],
             output_proto=[proto_output_dir_path],
-            additional_arguments=testcase.additional_arguments
+            allow_duplicates=testcase.allow_duplicates
         )
 
         # Sanitize and denoise the lines

xml_converter/integration_tests/src/testcase_loader.py

@@ -21,7 +21,7 @@ class Testcase:
     expected_stdout: List[str]
     expected_stderr: List[str]
     expected_returncode: int
-    additional_arguments: List[str]
+    allow_duplicates: bool
 
 
 ################################################################################
@@ -59,7 +59,7 @@ def load_testcase(path: str) -> Optional[Testcase]:
     # Load all of the input paths into either xml input or proto inputs
     xml_input_paths: List[str] = []
     proto_input_paths: List[str] = []
-    additional_arguments: List[str] = []
+    allow_duplicates: bool = False
     for pack_name, pack_type in data["input_paths"].items():
         if not isinstance(pack_name, str):
             print(f"Invalid pack name, expecting a string but got {pack_name}")
@@ -106,12 +106,12 @@ def load_testcase(path: str) -> Optional[Testcase]:
         print(f"Invalid Test, expecting string value for 'expected_returncode' in {path}")
         return None
 
-    if "additional_arguments" in data:
-        if not isinstance(data["additional_arguments"], str):
-            print(f"Invalid Test, expecting string value for 'additional_arguments' in {path}")
+    if "allow_duplicates" in data:
+        if not isinstance(data["allow_duplicates"], bool):
+            print(f"Invalid Test, expecting bool value for 'allow_duplicates' in {path}")
             return None
         else:
-            additional_arguments = to_lines(data["additional_arguments"])
+            allow_duplicates = data["allow_duplicates"]
 
     return Testcase(
         name=os.path.basename(path),
@@ -122,7 +122,7 @@ def load_testcase(path: str) -> Optional[Testcase]:
         expected_stdout=to_lines(data["expected_stdout"]),
         expected_stderr=to_lines(data["expected_stderr"]),
         expected_returncode=data["expected_returncode"],
-        additional_arguments=additional_arguments
+        allow_duplicates=allow_duplicates
     )
 
 

xml_converter/integration_tests/test_cases/proto_and_xml_input_allow_duplicates/testcase.yaml

@@ -3,10 +3,6 @@ input_paths:
     "pack2": "proto"
     "pack3": "xml"
 expected_stdout: |
-    The following top level categories were found in more than one pack
-    mycategory
-    mycategory
 expected_stderr: |
 expected_returncode: 0
-additional_arguments: |
-    --allow-duplicates
+allow_duplicates: true

xml_converter/src/packaging_xml.cpp

@@ -17,49 +17,54 @@ using namespace std;
 ////////////////////////////////////////////////////////////////////////////////
 
 unsigned int UNKNOWN_CATEGORY_COUNTER = 0;
-string parse_marker_categories(
+OptionalString parse_marker_categories(
     rapidxml::xml_node<>* node,
     map<string, Category>* marker_categories,
     Category* parent,
     vector<XMLError*>* errors,
     XMLReaderState* state,
     int depth = 0) {
+    OptionalString name = {
+        "",  // value
+        false,  // error
+    };
     if (get_node_name(node) == "MarkerCategory") {
-        string name;
-
         rapidxml::xml_attribute<>* name_attribute = find_attribute(node, "name");
         if (name_attribute == 0) {
             // TODO: This error should really be for the entire node not just the name
             errors->push_back(new XMLNodeNameError("Category attribute 'name' is missing so it cannot be properly referenced", node));
+            name.error = true;
 
             // TODO: Maybe fall back on display name slugification.
-            name = "UNKNOWN_CATEGORY_" + to_string(UNKNOWN_CATEGORY_COUNTER);
+            name.value = "UNKNOWN_CATEGORY_" + to_string(UNKNOWN_CATEGORY_COUNTER);
             UNKNOWN_CATEGORY_COUNTER++;
         }
         else {
-            name = lowercase(get_attribute_value(name_attribute));
+            name.value = lowercase(get_attribute_value(name_attribute));
         }
 
-        if (name == "") {
+        if (name.value == "") {
             errors->push_back(new XMLNodeNameError("Category attribute 'name' is an empty string so it cannot be properly referenced", node));
+            name.error = true;
             // TODO: Maybe fall back on display name slugification.
-            name = "UNKNOWN_CATEGORY_" + to_string(UNKNOWN_CATEGORY_COUNTER);
+            name.value = "UNKNOWN_CATEGORY_" + to_string(UNKNOWN_CATEGORY_COUNTER);
             UNKNOWN_CATEGORY_COUNTER++;
         }
 
         // Build the new category
         Category* category;
 
         // Create and initialize a new category if this one does not exist
-        auto existing_category_search = marker_categories->find(name);
+        auto existing_category_search = marker_categories->find(name.value);
         if (existing_category_search == marker_categories->end()) {
-            category = &(*marker_categories)[name];
+            category = &(*marker_categories)[name.value];
             category->parent = parent;
         }
         else {
             category = &existing_category_search->second;
             if (category->parent != parent) {
                 errors->push_back(new XMLNodeNameError("Category somehow has a different parent then it used to. This might be a bug in xml_converter", node));
+                name.error = true;
             }
         }
 
@@ -69,7 +74,7 @@ string parse_marker_categories(
         // based on the hashes of its name and its parents names.
         if (!category->menu_id_is_set) {
             Hash128 new_id;
-            new_id.update(name);
+            new_id.update(name.value);
 
             Category* next_node = parent;
             while (next_node != nullptr) {
@@ -87,7 +92,8 @@ string parse_marker_categories(
     }
     else {
         errors->push_back(new XMLNodeNameError("Unknown MarkerCategory Tag", node));
-        return "";
+        name.error = true;
+        return name;
     }
 }
 
@@ -214,7 +220,10 @@ set<string> parse_xml_file(string xml_filepath, const string marker_pack_root_di
 
     for (rapidxml::xml_node<>* node = root_node->first_node(); node; node = node->next_sibling()) {
         if (get_node_name(node) == "MarkerCategory") {
-            category_names.insert(parse_marker_categories(node, marker_categories, nullptr, &errors, &state));
+            OptionalString name = parse_marker_categories(node, marker_categories, nullptr, &errors, &state);
+            if (name.error == false) {
+                category_names.insert(name.value);
+            }
         }
         else if (get_node_name(node) == "POIs") {
             vector<Parseable*> temp_vector = parse_pois(node, marker_categories, &errors, &state);

xml_converter/src/string_helper.cpp

@@ -3,6 +3,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <iostream>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -314,3 +315,16 @@ std::string long_to_hex_string(uint64_t number) {
 
     return hex_string;
 }
+
+void combine_sets(
+    std::set<std::string>* set_a,
+    std::set<std::string>* set_b,
+    std::vector<std::string>* duplicates) {
+    for (string str : *set_a) {
+        if (auto search = set_b->find(str); search != set_b->end()) {
+            duplicates->push_back(str);
+        }
+        else
+            set_b->insert(str);
+    }
+}

xml_converter/src/string_helper.hpp

@@ -2,9 +2,15 @@
 
 #include <cstdint>
 #include <initializer_list>
+#include <set>
 #include <string>
 #include <vector>
 
+struct OptionalString {
+    std::string value;
+    bool error;
+};
+
 bool matches_any(std::string test, std::initializer_list<std::string> list);
 bool normalized_matches_any(std::string test, std::initializer_list<std::string> list);
 bool normalized_matches_any(std::string test, std::vector<std::string> list);
@@ -24,3 +30,4 @@ bool has_suffix(std::string const& fullString, std::string const& ending);
 std::string join_file_paths(const std::string& path_a, const std::string& path_b);
 
 std::string long_to_hex_string(uint64_t number);
+void combine_sets(std::set<std::string>* set_a, std::set<std::string>* set_b, std::vector<std::string>* duplicates);

xml_converter/src/xml_converter.cpp

@@ -144,16 +144,17 @@ void write_burrito_directory(
 void process_data(
     vector<string> input_taco_paths,
     vector<string> input_guildpoint_paths,
-
+    // If multiple inputs are found to have the same top level categories,
+    // The program will skip writing to output unless the below is true
+    bool allow_duplicates,
     // These will eventually have additional arguments for each output path to
     // allow for splitting out a single markerpack
     vector<string> output_taco_paths,
     vector<string> output_guildpoint_paths,
 
     // This is a special output path used for burrito internal use that splits
     // the guildpoint protobins by map id.
-    string output_split_guildpoint_dir,
-    bool allow_duplicates) {
+    string output_split_guildpoint_dir) {
     // All of the loaded pois and categories
     vector<Parseable*> parsed_pois;
     map<string, Category> marker_categories;
@@ -169,12 +170,7 @@ void process_data(
             input_taco_paths[i],
             &marker_categories,
             &parsed_pois);
-        for (string category_name : category_names) {
-            if (find(top_level_categories.begin(), top_level_categories.end(), category_name) != top_level_categories.end())
-                duplicate_categories.push_back(category_name);
-            else
-                top_level_categories.insert(category_name);
-        }
+        combine_sets(&category_names, &top_level_categories, &duplicate_categories);
     }
     auto end = chrono::high_resolution_clock::now();
     auto dur = end - begin;
@@ -189,23 +185,16 @@ void process_data(
             input_guildpoint_paths[i],
             &marker_categories,
             &parsed_pois);
-        for (string category_name : category_names) {
-            if (find(top_level_categories.begin(), top_level_categories.end(), category_name) != top_level_categories.end())
-                duplicate_categories.push_back(category_name);
-            else
-                top_level_categories.insert(category_name);
-        }
+        combine_sets(&category_names, &top_level_categories, &duplicate_categories);
     }
 
-    if (duplicate_categories.size() > 0) {
+    if (duplicate_categories.size() > 0 && allow_duplicates != true) {
+        cout << "Did not write due to duplicates in categories. If you want to bypass this, use '--allow-duplicates'" << endl;
         cout << "The following top level categories were found in more than one pack" << endl;
         for (size_t i = 0; i < duplicate_categories.size(); i++) {
             cout << duplicate_categories[i] << endl;
         }
-        if (allow_duplicates != true) {
-            cout << "Did not write due to duplicates in categories. If you want to bypass this, use '--allow-duplicates'" << endl;
-            return;
-        }
+        return;
     }
 
     // Write all of the xml taco paths
@@ -252,7 +241,7 @@ int main(int argc, char* argv[]) {
     vector<string> output_taco_paths;
     vector<string> input_guildpoint_paths;
     vector<string> output_guildpoint_paths;
-    bool allow_duplicates;
+    bool allow_duplicates = false;
 
     // Typically "~/.local/share/godot/app_userdata/Burrito/protobins" for
     // converting from xml markerpacks to internal protobuf files.
@@ -281,9 +270,16 @@ int main(int argc, char* argv[]) {
         }
         else if (!strcmp(argv[i], "--allow-duplicates")) {
             allow_duplicates = true;
+            arg_target = nullptr;
         }
         else {
-            arg_target->push_back(argv[i]);
+            if (arg_target != nullptr) {
+                arg_target->push_back(argv[i]);
+            }
+            else {
+                cout << "Unknown argument " << argv[i] << endl;
+                return -1;
+            }
         }
     }
 
@@ -300,10 +296,10 @@ int main(int argc, char* argv[]) {
     process_data(
         input_taco_paths,
         input_guildpoint_paths,
+        allow_duplicates,
         output_taco_paths,
         output_guildpoint_paths,
-        output_split_guildpoint_dir,
-        allow_duplicates);
+        output_split_guildpoint_dir);
 
     return 0;
 }