Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command line parser #378

Open
wants to merge 13 commits into
base: xml_converter
Choose a base branch
from
Open
20 changes: 9 additions & 11 deletions xml_converter/integration_tests/run_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,19 @@


def run_xml_converter(
allow_duplicates: Optional[bool] = None,
input_xml: Optional[List[str]] = None,
output_xml: Optional[List[str]] = None,
input_proto: Optional[List[str]] = None,
output_proto: Optional[List[str]] = None,
split_output_proto: Optional[str] = None,
allow_duplicates: Optional[bool] = None,
split_by_map_id: Optional[bool] = None
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

random nit, in python we can have trailing commas on arguments. In C++ we cannot. When args are split onto individual lines like this I like to have a trailing comma when possible.

) -> Tuple[str, str, int]:

# Build the command to execute the C++ program with the desired function and arguments
cmd: List[str] = [xml_converter_binary_path]

if allow_duplicates:
cmd += ["--allow-duplicates"]
if input_xml:
cmd += ["--input-taco-path"] + input_xml
if output_xml:
Expand All @@ -34,10 +36,8 @@ def run_xml_converter(
cmd += ["--input-guildpoint-path"] + input_proto
if output_proto:
cmd += ["--output-guildpoint-path"] + output_proto
if split_output_proto:
cmd += ["--output-split-guildpoint-path"] + [split_output_proto]
if allow_duplicates:
cmd += ["--allow-duplicates"]
if split_by_map_id:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the ordering, does this mean that if split-by-map-id is specified then only the proto will be split by map id? If so that's fine for now but you should make a new issue detailing the problems that arise from this and how one might fix them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct for now. This would be the next step in adjusting the test harness so that each input and output can be configured. My thought was to make a dictionary of adjustments so that they can be processed as a group for each type.

example:

configurations: 
    "xml" : "split_by_category"
   "proto" : "split_by_map_id"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good first thought, but assumes that all xml inputs want one argument, and the same argument. Instead just transform the inputs field from a Dict[str,str] to a Dict[str, ConfigData] where ConfigData is something like {"type": "xml", "split_by_category": true}. This will allow the inputs to be specified in an intuitive manner to anyone reading the test config, and will also allow for test inputs to nearly mirror the new cli flags.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link the new issue made for this here?

cmd += ["--split-by-map-id"]

# Run the C++ program and capture its output
result = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
Expand Down Expand Up @@ -126,10 +126,7 @@ def rebuild_xml_converter_binary() -> None:
line_patterns_to_ignore = [
r"^Loading taco pack .*$",
r"^Loading guildpoint pack .*$",
r"^The taco parse function took [0-9]+ milliseconds to run$",
r"^The xml write function took [0-9]+ milliseconds to run$",
r"^The protobuf read function took [0-9]+ milliseconds to run$",
r"^The protobuf write function took [0-9]+ milliseconds to run$",
r"^The .+? function took [0-9]+ milliseconds to run$",
r"^$"
]

Expand Down Expand Up @@ -186,11 +183,12 @@ def main() -> bool:
output_proto_paths = [proto_output_dir_path]

rawstdout, rawstderr, returncode = run_xml_converter(
allow_duplicates=testcase.allow_duplicates,
input_xml=testcase.xml_input_paths,
input_proto=testcase.proto_input_paths,
output_xml=output_xml_paths,
output_proto=output_proto_paths,
allow_duplicates=testcase.allow_duplicates
split_by_map_id=testcase.split_by_map_id
)

# Sanitize and denoise the lines
Expand Down
12 changes: 11 additions & 1 deletion xml_converter/integration_tests/src/testcase_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class Testcase:
expected_stderr: List[str]
expected_returncode: int
allow_duplicates: bool
split_by_map_id: bool


################################################################################
Expand Down Expand Up @@ -60,6 +61,7 @@ def load_testcase(path: str) -> Optional[Testcase]:
xml_input_paths: List[str] = []
proto_input_paths: List[str] = []
allow_duplicates: bool = False
split_by_map_id: 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}")
Expand Down Expand Up @@ -126,6 +128,13 @@ def load_testcase(path: str) -> Optional[Testcase]:
else:
allow_duplicates = data["allow_duplicates"]

if "split_by_map_id" in data:
if not isinstance(data["split_by_map_id"], bool):
print(f"Invalid Test, expecting bool value for 'split_by_map_id' in {path}")
return None
else:
split_by_map_id = data["split_by_map_id"]

return Testcase(
name=os.path.basename(path),
xml_input_paths=xml_input_paths,
Expand All @@ -135,7 +144,8 @@ 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"],
allow_duplicates=allow_duplicates
allow_duplicates=allow_duplicates,
split_by_map_id=split_by_map_id
)


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<OverlayData>
<MarkerCategory DisplayName="My Category" Name="mycategory">
</MarkerCategory>

<POIs>
<POI Type="mycategory" XPos="100" YPos="200" ZPos="300" MapID="0" />
<POI Type="mycategory" XPos="110" YPos="210" ZPos="310" MapID="1" />
<POI Type="mycategory" XPos="120" YPos="220" ZPos="320" MapID="5" />
<POI Type="mycategory" XPos="130" YPos="230" ZPos="330" MapID="2147483647" />
<POI Type="mycategory" XPos="140" YPos="240" ZPos="340" MapID="50" />
</POIs>
</OverlayData>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- old
+++ new
@@ -0,0 +1,11 @@
+category {
+  name: "My Category"
+  icon {
+    position {
+      x: 100
+      y: 200
+      z: 300
+    }
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Binary file not shown.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- old
+++ new
@@ -0,0 +1,12 @@
+category {
+  name: "My Category"
+  icon {
+    map_id: 1
+    position {
+      x: 110
+      y: 210
+      z: 310
+    }
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Binary file not shown.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- old
+++ new
@@ -0,0 +1,12 @@
+category {
+  name: "My Category"
+  icon {
+    map_id: 2147483647
+    position {
+      x: 130
+      y: 230
+      z: 330
+    }
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Binary file not shown.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- old
+++ new
@@ -0,0 +1,12 @@
+category {
+  name: "My Category"
+  icon {
+    map_id: 5
+    position {
+      x: 120
+      y: 220
+      z: 320
+    }
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Binary file not shown.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full Diff
--- old
+++ new
@@ -0,0 +1,12 @@
+category {
+  name: "My Category"
+  icon {
+    map_id: 50
+    position {
+      x: 140
+      y: 240
+      z: 340
+    }
+  }
+  id: "(\350\314\006\302\223^\226"
+}

Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<OverlayData>
<MarkerCategory DisplayName="My Category" ID="KOjMBsKTXpY=" Name="mycategory"/>
<POIs>
<POI Type="mycategory" MapID="0" XPos="100.000000" YPos="200.000000" ZPos="300.000000"/>
<POI Type="mycategory" MapID="1" XPos="110.000000" YPos="210.000000" ZPos="310.000000"/>
<POI Type="mycategory" MapID="5" XPos="120.000000" YPos="220.000000" ZPos="320.000000"/>
<POI Type="mycategory" MapID="2147483647" XPos="130.000000" YPos="230.000000" ZPos="330.000000"/>
<POI Type="mycategory" MapID="50" XPos="140.000000" YPos="240.000000" ZPos="340.000000"/>
</POIs>
</OverlayData>

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
input_paths:
"pack": "xml"
expected_stdout: |
expected_stderr: |
expected_returncode: 0
split_by_map_id: True
92 changes: 92 additions & 0 deletions xml_converter/src/argument_parser.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
#include "argument_parser.hpp"

#include <cstring>
#include <iostream>
#include <map>

using namespace std;

// Default constructor for the class
PathConfig::PathConfig()
: type(BehaviorType::NONE),
format(MarkerFormat::NONE),
path("DEFAULT"),
split_by_map_id(false) {
}

PathConfig::PathConfig(BehaviorType type, MarkerFormat format, std::string path, bool split_by_map_id)
: type(type), format(format), path(path), split_by_map_id(split_by_map_id) {
}

ArgumentConfig::ArgumentConfig(BehaviorType type, MarkerFormat format)
: type(type), format(format) {
}

////////////////////////////////////////////////////////////////////////////////
// parse_arguments
//
// Processes all of the command line data into a format the internal functions
// want to receive.
////////////////////////////////////////////////////////////////////////////////
ParsedArguments parse_arguments(int argc, char* argv[]) {
vector<PathConfig> path_configs;
map<string, ArgumentConfig> arg_map = {
{"--input-taco-path", ArgumentConfig(BehaviorType::IMPORT, MarkerFormat::XML)},
{"--output-taco-path", ArgumentConfig(BehaviorType::EXPORT, MarkerFormat::XML)},
{"--input-guildpoint-path", ArgumentConfig(BehaviorType::IMPORT, MarkerFormat::GUILDPOINT)},
{"--output-guildpoint-path", ArgumentConfig(BehaviorType::EXPORT, MarkerFormat::GUILDPOINT)}};
ParsedArguments parsed_arguments;
BehaviorType type = BehaviorType::NONE;
MarkerFormat format = MarkerFormat::NONE;
bool split_by_map_id = false;
string current_argument;
vector<string> current_paths;

for (int i = 1; i < argc; i++) {
auto it = arg_map.find(argv[i]);
if (it != arg_map.end()) {
if (!current_paths.empty()) {
for (const string& path : current_paths) {
path_configs.emplace_back(type, format, path, split_by_map_id);
}
current_paths.clear();
}
else if (type != BehaviorType::NONE) {
cerr << "Error: Expected a path to a directory after " << current_argument << endl;
return {};
}
current_argument = argv[i];
type = it->second.type;
format = it->second.format;
// All flags must be set to default value
split_by_map_id = false;
}
else if (!strcmp(argv[i], "--allow-duplicates")) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets wrap argv[i] as a std::string and use == instead of strcmp.

parsed_arguments.allow_duplicates = true;
}
else if (!strcmp(argv[i], "--split-by-map-id")) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use == instead of strcmp.

if (type == BehaviorType::IMPORT) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets make this != EXPORT

cerr << "Error: --split-by-map-id cannot be used after an input argument" << endl;
return {};
}
split_by_map_id = true;
}
else {
current_paths.push_back(argv[i]);
}
}

if (!current_paths.empty()) {
for (const auto& path : current_paths) {
path_configs.emplace_back(type, format, path, split_by_map_id);
}
}
else if (type != BehaviorType::NONE) {
cerr << "Error: Expected a path to a directory after " << current_argument << endl;
return {};
}

parsed_arguments.path_configs = path_configs;
parsed_arguments.is_valid = true;
return parsed_arguments;
}
47 changes: 47 additions & 0 deletions xml_converter/src/argument_parser.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#pragma once

#include <string>
#include <vector>

// Defines what behavior is expected i.e. (Read/Import or Write/Export)
enum class BehaviorType {
IMPORT,
EXPORT,
NONE,
};

// Defines what format the data is expected to be in
enum class MarkerFormat {
XML,
GUILDPOINT,
NONE,
};

class PathConfig {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this classname elsewhere in the code makes me realize that this name is not very good. This is not configuration information about the path, the path is part of the configuration for a marker pack input/output.

public:
BehaviorType type;
MarkerFormat format;
std::string path;
bool split_by_map_id = false;

PathConfig();

PathConfig(BehaviorType type, MarkerFormat format, std::string path, bool split_by_map_id = false);
};

class ArgumentConfig {
public:
BehaviorType type;
MarkerFormat format;

ArgumentConfig(BehaviorType type, MarkerFormat format);
};
Comment on lines +32 to +38
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it is intended only for use inside of argument_parser.cpp as such you dont need to include it inside of the header file and you can just define and implement it in the cpp


class ParsedArguments {
public:
std::vector<PathConfig> path_configs;
bool allow_duplicates = false;
bool is_valid = false;
};

ParsedArguments parse_arguments(int argc, char* argv[]);
Loading