diff --git a/PostMortem.md b/PostMortem.md index be4bb3e..8785a1e 100644 --- a/PostMortem.md +++ b/PostMortem.md @@ -7,3 +7,16 @@ These issues were related to the deployment of the application. * unused capture in one of the lambdas. Again, `g++` was not reporting this warning, but reviewer's macos clang warned about it and with `-Werror` caused compilation to fail. Fixes to the above are included in the `appeals` tag. + +# Issues reported as part of the Review phase + +Three issues were reported. Quote: + +1. If the output directory does not exist, the CLI should create it. +2. There's no way to provide a list of excluded tags as discussed in the forum. +3. Your CLI does not handle invalid input files gracefully. It should catch the error and show a meaningful message. Right now, it throws something like the error below and crashes: +`[2020-11-23 20:26:16.724] [error] Bad XML: (1): expected <` + +For the first two, fixes are included in the `final` tag. + +As for the third one, this is the implemented graceful exit. Error notification is presented with the timestamp, `Bad XML` is clearly part of the message, and the exact failure reason reported by the used 3rd party XML parser is quoted verbatim. Arguably this one is unjust. diff --git a/solution/app/src/filesystem.cpp b/solution/app/src/filesystem.cpp index 1b3c8e9..633ff55 100644 --- a/solution/app/src/filesystem.cpp +++ b/solution/app/src/filesystem.cpp @@ -3,6 +3,7 @@ #include #include +#include #if defined __cpp_lib_filesystem #include @@ -39,12 +40,21 @@ Either check_directory(std::string const & odir) auto const path = fs::path(odir); std::error_code ec; - if (not fs::is_directory(path, ec)) + if (fs::exists(path, ec)) { - return rv_type::leftOf(fmt::format("{}: {}", odir, ec.message())); + if (not fs::is_directory(path, ec)) + { + // interestingly, ec is set to Success + return rv_type::leftOf(fmt::format("{}: Not a directory", odir)); + } } else { - return rv_type::rightOf(odir); + if (not fs::create_directories(path, ec)) + { + return rv_type::leftOf(fmt::format("Failed to create {}: {}", odir, ec.message())); + } } + + return rv_type::rightOf(odir); } diff --git a/solution/app/src/main.cpp b/solution/app/src/main.cpp index 2602c75..f026cec 100644 --- a/solution/app/src/main.cpp +++ b/solution/app/src/main.cpp @@ -6,6 +6,8 @@ #include #include +#include +#include /* @@ -50,10 +52,12 @@ int main(int argc, char **argv) bool verbose = false; bool debug = false; int json_indent = -1; + std::vector to_ignore; auto cli = ( clipp::required("--input", "-i").doc("Input XML file") & clipp::value("XML data file to read from", xml_ifname), clipp::required("--output-dir", "-o").doc("Destination folder where output files are to be created") & clipp::value("Destination folder", out_dir), + clipp::option("--ignore-tags", "-x").doc("Specify tags to ignore") & clipp::values("List of tags to ignore", to_ignore), clipp::option("--root-name").doc("Name for the root node to use, if enabled") & clipp::value("Name for the root node=" + root_name, root_name), ( clipp::option("--skip-root").set(skip_root, true) | @@ -86,10 +90,12 @@ int main(int argc, char **argv) {"json_indent", json_indent} }; + std::unordered_set const to_ignore_unique(to_ignore.cbegin(), to_ignore.cend()); + /* * Attempt to do the actual work */ - auto rv = maybe_work(xml_ifname, out_dir, cli_params); + auto rv = maybe_work(xml_ifname, out_dir, to_ignore_unique, cli_params); spdlog::info("Done"); diff --git a/solution/app/src/parser.cpp b/solution/app/src/parser.cpp index 29bf3c5..6f13525 100644 --- a/solution/app/src/parser.cpp +++ b/solution/app/src/parser.cpp @@ -43,7 +43,10 @@ using boost::property_tree::ptree; * node will or will not be included in final result, and what the custom name * shall be. By default this root node is skipped. */ -tags_tree_t collect_tag_stats(ptree && pt, cli_params_t const & cli_params) +tags_tree_t collect_tag_stats( + ptree && pt, + std::unordered_set const & to_ignore, + cli_params_t const & cli_params) { tags_tree_t tags; std::queue> nodes; @@ -86,7 +89,10 @@ tags_tree_t collect_tag_stats(ptree && pt, cli_params_t const & cli_params) } // update known children and their counts - unique_children.insert(child_name); + if (to_ignore.count(child_name) == 0) + { + unique_children.insert(child_name); + } nodes.emplace(child_name, child_subtree); } @@ -103,12 +109,21 @@ tags_tree_t collect_tag_stats(ptree && pt, cli_params_t const & cli_params) tags.erase(params::root_name(cli_params)); } + // drop tags to ignore + for (auto const & tag : to_ignore) + { + tags.erase(tag); + } + return tags; } Either -parse_xml_file(std::ifstream & ifile, cli_params_t const & cli_params) +parse_xml_file( + std::ifstream & ifile, + std::unordered_set const & to_ignore, + cli_params_t const & cli_params) { using rv_type = Either; @@ -119,10 +134,10 @@ parse_xml_file(std::ifstream & ifile, cli_params_t const & cli_params) // call boost's XML parser read_xml(ifile, pt); } - catch(boost::property_tree::xml_parser::xml_parser_error & ex) + catch (boost::property_tree::xml_parser::xml_parser_error & ex) { return rv_type::leftOf(fmt::format("Bad XML: {}", ex.what())); } - return rv_type::rightOf(collect_tag_stats(std::move(pt), cli_params)); + return rv_type::rightOf(collect_tag_stats(std::move(pt), to_ignore, cli_params)); } diff --git a/solution/app/src/parser.hpp b/solution/app/src/parser.hpp index c0772af..fffd5aa 100644 --- a/solution/app/src/parser.hpp +++ b/solution/app/src/parser.hpp @@ -9,6 +9,7 @@ #include #include #include +#include /* @@ -45,7 +46,10 @@ using tags_tree_t = std::unordered_map; */ [[nodiscard]] neither::Either -parse_xml_file(std::ifstream & ifile, cli_params_t const & cli_params); +parse_xml_file( + std::ifstream & ifile, + std::unordered_set const & to_ignore, + cli_params_t const & cli_params); #endif /* APP_SRC_PARSER_HPP_ */ diff --git a/solution/app/src/work.cpp b/solution/app/src/work.cpp index 52dc522..7656528 100644 --- a/solution/app/src/work.cpp +++ b/solution/app/src/work.cpp @@ -8,6 +8,7 @@ #include "spdlog/spdlog.h" #include +#include /* @@ -45,9 +46,10 @@ int work( std::ifstream & ifile, std::string const & odir, + std::unordered_set const & to_ignore, cli_params_t const & cli_params) { - int rv = parse_xml_file(ifile, cli_params) + int rv = parse_xml_file(ifile, to_ignore, cli_params) .rightFlatMap( [&cli_params](tags_tree_t && tags) { @@ -70,21 +72,22 @@ int work( int maybe_work( std::string const & ifname, std::string const & odir, + std::unordered_set const & to_ignore, cli_params_t const & cli_params) { int rv = open_file_r(ifname) .leftMap(error_printer) .rightMap( - [&odir, &cli_params](std::ifstream && ifile) + [&odir, &to_ignore, &cli_params](std::ifstream && ifile) { int rv = check_directory(odir) .leftMap(error_printer) .rightMap( - [&ifile, &cli_params](std::string const & odir) + [&ifile, &to_ignore, &cli_params](std::string const & odir) { - work(ifile, odir, cli_params); + int const rv = work(ifile, odir, to_ignore, cli_params); - return 0; + return rv; } ).join(); diff --git a/solution/app/src/work.hpp b/solution/app/src/work.hpp index e3ae5df..a3b68dc 100644 --- a/solution/app/src/work.hpp +++ b/solution/app/src/work.hpp @@ -6,6 +6,7 @@ #include "cli_params.hpp" #include +#include /* @@ -19,6 +20,7 @@ int maybe_work( std::string const & ifname, std::string const & odir, + std::unordered_set const & to_ignore, cli_params_t const & cli_params);