Commit 4b4d3fcb authored by Samir Halilcevic's avatar Samir Halilcevic

Integrate review feedback

parent 9fe1e9f4
......@@ -23,6 +23,8 @@ is based on [Keep a Changelog](https://keepachangelog.com).
- The output of `--dump-config` now only contains CAF options from loaded
modules. Previously, it also included options from modules that were not
loaded.
- The CLI argument format now supports a space separator when using long
argument names (e.g. `--foo bar`)
### Fixed
......
......@@ -19,6 +19,21 @@ namespace caf {
class CAF_CORE_EXPORT config_option {
public:
// -- member types -----------------------------------------------------------
/// An iterator over CLI arguments.
using argument_iterator = std::vector<std::string>::const_iterator;
/// Stores the result of a find operation. The option sets `begin == end`
/// if the operation could not find a match.
struct find_result {
/// The begin of the matched range.
argument_iterator begin;
/// The end of the matched range.
argument_iterator end;
/// The value for the config option.
std::string_view value;
};
/// Custom vtable-like struct for delegating to type-specific functions and
/// storing type-specific information shared by several config options.
......@@ -92,6 +107,10 @@ public:
/// Returns whether the category is optional for CLI options.
bool has_flat_cli_name() const noexcept;
/// Tries to find this option by its long name in `[first, last)`.
find_result find_by_long_name(argument_iterator first,
argument_iterator last) const noexcept;
private:
std::string_view buf_slice(size_t from, size_t to) const noexcept;
......@@ -104,13 +123,4 @@ private:
mutable void* value_;
};
/// Finds `config_option` string with a matching long name in the argument list
/// [`first`, `last`). Returns an `iterator` to the matching location and a
/// `string_view` of the option value if the entry is found, or a `iterator`
/// to `last` with an empty `string_view` otherwise.
std::pair<std::vector<std::string>::const_iterator, std::string_view>
find_by_long_name(const config_option& x,
std::vector<std::string>::const_iterator first,
std::vector<std::string>::const_iterator last);
} // namespace caf
......@@ -462,19 +462,15 @@ std::pair<error, std::string>
actor_system_config::extract_config_file_path(string_list& args) {
auto ptr = custom_options_.qualified_name_lookup("global.config-file");
CAF_ASSERT(ptr != nullptr);
auto [i, path] = find_by_long_name(*ptr, args.begin(), args.end());
if (i == args.end()) {
auto [first, last, path] = ptr->find_by_long_name(args.begin(), args.end());
if (first == args.end()) {
return {none, std::string{}};
} else if (path.empty()) {
return {make_error(pec::missing_argument, "no argument to --config-file"),
std::string{}};
} else {
auto path_str = std::string{path.begin(), path.end()};
if (i->find('=') != std::string::npos) {
args.erase(i);
} else {
args.erase(i, i + 2);
}
auto path_str = std::string{path};
args.erase(first, last);
config_value val{path_str};
if (auto err = ptr->sync(val); !err) {
put(content, "config-file", std::move(val));
......
......@@ -134,11 +134,10 @@ std::string_view config_option::buf_slice(size_t from,
}
// TODO: consider using `config_option_set` and deprecating this
std::pair<std::vector<std::string>::const_iterator, std::string_view>
find_by_long_name(const config_option& x,
std::vector<std::string>::const_iterator first,
std::vector<std::string>::const_iterator last) {
auto long_name = x.long_name();
config_option::find_result config_option::find_by_long_name(
config_option::argument_iterator first,
config_option::argument_iterator last) const noexcept {
auto argument_name = long_name();
for (; first != last; ++first) {
std::string_view str{*first};
// Make sure this is a long option starting with "--".
......@@ -146,24 +145,24 @@ find_by_long_name(const config_option& x,
continue;
str.remove_prefix(2);
// Make sure we are dealing with the right key.
if (!starts_with(str, long_name))
if (!starts_with(str, argument_name))
continue;
str.remove_prefix(long_name.size());
str.remove_prefix(argument_name.size());
// check for flag
if (x.is_flag() && str.empty()) {
return {first, str};
if (is_flag() && str.empty()) {
return {first, first + 1, str};
} else if (starts_with(str, "=")) {
// Remove leading '=' and return the value.
str.remove_prefix(1);
return {first, str};
return {first, first + 1, str};
} else if (auto val = first + 1; str.empty() && val != last) {
// Get the next argument the value
return {first, std::string_view{*val}};
return {first, first + 2, std::string_view{*val}};
} else {
continue;
}
}
return {first, std::string_view{}};
return {first, first, std::string_view{}};
}
} // namespace caf
......@@ -149,21 +149,6 @@ CAF_TEST(file input overrides defaults but CLI args always win) {
CHECK_EQ(content(cfg), res);
}
CAF_TEST(parsing - with config file cli option) {
MESSAGE("Try opening non-existent config file");
cfg.clear();
auto err = cfg.parse(string_list{"--config-file=test-me"});
CHECK_EQ(err, caf::sec::cannot_open_file);
CHECK(cfg.remainder.empty());
CHECK_EQ(get_or(cfg, "config-file", ""), "test-me");
cfg.clear();
err = cfg.parse(string_list{"--config-file", "test-me"});
CHECK_EQ(err, caf::sec::cannot_open_file);
CHECK(cfg.remainder.empty());
CHECK_EQ(get_or(cfg, "config-file", ""), "test-me");
}
// Checks whether both a synced variable and the corresponding entry in
// content(cfg) are equal to `value`.
#define CHECK_SYNCED(var, ...) \
......
......@@ -343,12 +343,12 @@ CAF_TEST(find by long opt) {
auto needle = make_config_option<std::string>("?foo"sv, "bar,b"sv,
"test option"sv);
auto check = [&](std::vector<string> args, bool found_opt, bool has_opt) {
auto res = find_by_long_name(needle, std::begin(args), std::end(args));
CHECK_EQ(res.first != std::end(args), found_opt);
auto res = needle.find_by_long_name(std::begin(args), std::end(args));
CHECK_EQ(res.begin != std::end(args), found_opt);
if (has_opt)
CHECK_EQ(res.second, "val2");
CHECK_EQ(res.value, "val2");
else
CHECK(res.second.empty());
CHECK(res.value.empty());
};
// Well formed, find val2.
check({"--foo=val1", "--bar=val2", "--baz=val3"}, true, true);
......
......@@ -109,7 +109,7 @@ CAF_TEST(parse with ref syncing) {
CHECK_EQ(get_as<int>(cfg, "foo.i"), 42);
}
CAF_TEST(Long format for flags) {
CAF_TEST(long format for flags) {
auto foo_b = false;
opts.add<bool>(foo_b, "foo", "b,b", "");
settings cfg;
......
find_package(Python COMPONENTS Interpreter)
add_test(
NAME "robot-config-read-config-file"
COMMAND
${Python_EXECUTABLE}
-m robot
--variable BINARY_PATH:$<TARGET_FILE:hello_world>
"${CMAKE_CURRENT_SOURCE_DIR}/config/read-config-file.robot")
add_test(
NAME "robot-config-dump-config"
COMMAND
......
*** Settings ***
Documentation A test suite for --config-file.
Library OperatingSystem
Library Process
*** Variables ***
${BINARY_PATH} /path/to/the/test/binary
*** Test Cases ***
Test --config-file argument
[Documentation] Binary with a --config-file option tries to open the provided file.
[Tags] Config
Run Process ${BINARY_PATH} --config-file\=test-me.cfg stderr=output
${error_output}= Get File output
Should Contain ${error_output} cannot_open_file("test-me.cfg")
Run Process ${BINARY_PATH} --config-file test-me.cfg stderr=output
${error_output}= Get File output
Should Contain ${error_output} cannot_open_file("test-me.cfg")
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment