Skip to content

Commit

Permalink
Issue 1086 (#1087)
Browse files Browse the repository at this point in the history
Fixes issue #1086.

In the default_val enums of uint8_t would read in as a string as they
could be converted to a string. This worked ok for normal values, but
when a check was added for specific strings, it caused an error when the
default_val was added. This PR fixes the issue.

The builder for coverage was updated to CMake 3.31 (by github), this
triggered an error in the coverage tool script. This led to updating
that script, which led to uncovering some missing coverage, which led to
additional tests, which led to some issues around single element tuples
support from #1081, which led to a few other issues that came up in the
to_string operation and templates.

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
  • Loading branch information
3 people authored Nov 18, 2024
1 parent d9473a4 commit 3539bd1
Show file tree
Hide file tree
Showing 8 changed files with 705 additions and 80 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ jobs:
lcov --list coverage.info
working-directory: build

- uses: codecov/codecov-action@v4
- uses: codecov/codecov-action@v5
with:
files: build/coverage.info
functionalities: fixes
Expand Down
684 changes: 621 additions & 63 deletions cmake/CodeCoverage.cmake

Large diffs are not rendered by default.

20 changes: 19 additions & 1 deletion include/CLI/Option.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -776,10 +776,28 @@ class Option : public OptionBase<Option> {
_validate_results(results_);
current_option_state_ = old_option_state;
}
} catch(const CLI::Error &) {
} catch(const ValidationError &err) {
// this should be done
results_ = std::move(old_results);
current_option_state_ = old_option_state;
// try an alternate way to convert
std::string alternate = detail::value_string(val);
if(!alternate.empty() && alternate != val_str) {
return default_val(alternate);
}

throw ValidationError(get_name(),
std::string("given default value does not pass validation :") + err.what());
} catch(const ConversionError &err) {
// this should be done
results_ = std::move(old_results);
current_option_state_ = old_option_state;

throw ConversionError(
get_name(), std::string("given default value(\"") + val_str + "\") produces an error : " + err.what());
} catch(const CLI::Error &) {
results_ = std::move(old_results);
current_option_state_ = old_option_state;
throw;
}
results_ = std::move(old_results);
Expand Down
29 changes: 15 additions & 14 deletions include/CLI/TypeTools.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -279,9 +279,8 @@ struct is_mutable_container<
// check to see if an object is a mutable container (fail by default)
template <typename T, typename _ = void> struct is_readable_container : std::false_type {};

/// type trait to test if a type is a container meaning it has a value_type, it has an iterator, a clear, and an end
/// methods and an insert function. And for our purposes we exclude std::string and types that can be constructed from
/// a std::string
/// type trait to test if a type is a container meaning it has a value_type, it has an iterator, and an end
/// method.
template <typename T>
struct is_readable_container<
T,
Expand Down Expand Up @@ -323,7 +322,8 @@ struct type_count_base<T,
/// the base tuple size
template <typename T>
struct type_count_base<T, typename std::enable_if<is_tuple_like<T>::value && !is_mutable_container<T>::value>::type> {
static constexpr int value{std::tuple_size<typename std::remove_reference<T>::type>::value};
static constexpr int value{// cppcheck-suppress unusedStructMember
std::tuple_size<typename std::decay<T>::type>::value};
};

/// Type count base for containers is the type_count_base of the individual element
Expand All @@ -341,13 +341,13 @@ auto to_string(T &&value) -> decltype(std::forward<T>(value)) {
template <typename T,
enable_if_t<std::is_constructible<std::string, T>::value && !std::is_convertible<T, std::string>::value,
detail::enabler> = detail::dummy>
std::string to_string(const T &value) {
std::string to_string(T &&value) {
return std::string(value); // NOLINT(google-readability-casting)
}

/// Convert an object to a string (streaming must be supported for that type)
template <typename T,
enable_if_t<!std::is_convertible<std::string, T>::value && !std::is_constructible<std::string, T>::value &&
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
is_ostreamable<T>::value,
detail::enabler> = detail::dummy>
std::string to_string(T &&value) {
Expand All @@ -360,32 +360,33 @@ std::string to_string(T &&value) {

/// Print tuple value string for tuples of size ==1
template <typename T,
enable_if_t<!std::is_convertible<std::string, T>::value && !std::is_constructible<std::string, T>::value &&
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
!is_ostreamable<T>::value && is_tuple_like<T>::value && type_count_base<T>::value == 1,
detail::enabler> = detail::dummy>
inline std::string to_string(T &&value);

/// Print tuple value string for tuples of size > 1
template <typename T,
enable_if_t<!std::is_convertible<std::string, T>::value && !std::is_constructible<std::string, T>::value &&
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
!is_ostreamable<T>::value && is_tuple_like<T>::value && type_count_base<T>::value >= 2,
detail::enabler> = detail::dummy>
inline std::string to_string(T &&value);

/// If conversion is not supported, return an empty string (streaming is not supported for that type)
template <
typename T,
enable_if_t<!std::is_constructible<std::string, T>::value && !is_ostreamable<T>::value &&
!is_readable_container<typename std::remove_const<T>::type>::value && !is_tuple_like<T>::value,
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
!is_ostreamable<T>::value && !is_readable_container<typename std::remove_const<T>::type>::value &&
!is_tuple_like<T>::value,
detail::enabler> = detail::dummy>
inline std::string to_string(T &&) {
return {};
}

/// convert a readable container to a string
template <typename T,
enable_if_t<!std::is_constructible<std::string, T>::value && !is_ostreamable<T>::value &&
is_readable_container<T>::value,
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
!is_ostreamable<T>::value && is_readable_container<T>::value,
detail::enabler> = detail::dummy>
inline std::string to_string(T &&variable) {
auto cval = variable.begin();
Expand Down Expand Up @@ -413,7 +414,7 @@ inline typename std::enable_if<(I < type_count_base<T>::value), std::string>::ty

/// Print tuple value string for tuples of size ==1
template <typename T,
enable_if_t<!std::is_convertible<std::string, T>::value && !std::is_constructible<std::string, T>::value &&
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
!is_ostreamable<T>::value && is_tuple_like<T>::value && type_count_base<T>::value == 1,
detail::enabler>>
inline std::string to_string(T &&value) {
Expand All @@ -422,7 +423,7 @@ inline std::string to_string(T &&value) {

/// Print tuple value string for tuples of size > 1
template <typename T,
enable_if_t<!std::is_convertible<std::string, T>::value && !std::is_constructible<std::string, T>::value &&
enable_if_t<!std::is_convertible<T, std::string>::value && !std::is_constructible<std::string, T>::value &&
!is_ostreamable<T>::value && is_tuple_like<T>::value && type_count_base<T>::value >= 2,
detail::enabler>>
inline std::string to_string(T &&value) {
Expand Down
23 changes: 23 additions & 0 deletions tests/AppTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,19 @@ TEST_CASE_METHOD(TApp, "TupleDefault", "[app]") {
CHECK(pr == pr2);
}

TEST_CASE_METHOD(TApp, "TupleDefaultSingle", "[app]") {
std::tuple<std::string> pr{"test_tuple"};
auto *opt = app.add_option("-i", pr)->expected(0, 1);
args = {"-i"};
run();
CHECK(app.count("-i") == 1u);

std::tuple<std::string> pr2{"total3"};
opt->default_val(pr2);
run();
CHECK(pr == pr2);
}

TEST_CASE_METHOD(TApp, "TupleComplex", "[app]") {
std::tuple<double, std::string, int, std::pair<std::string, std::string>> pr{57.5, "test", 5, {"total", "total2"}};
auto *opt = app.add_option("-i", pr)->expected(0, 4);
Expand All @@ -518,6 +531,16 @@ TEST_CASE_METHOD(TApp, "TupleComplex", "[app]") {
CHECK(pr == pr2);
}

TEST_CASE_METHOD(TApp, "invalidDefault", "[app]") {
int pr{5};
auto *opt = app.add_option("-i", pr)
->expected(1)
->multi_option_policy(CLI::MultiOptionPolicy::Throw)
->delimiter(',')
->force_callback();
CHECK_THROWS(opt->default_val("4,6,2,8"));
}

TEST_CASE_METHOD(TApp, "TogetherInt", "[app]") {
int i{0};
app.add_option("-i,--int", i);
Expand Down
2 changes: 1 addition & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ endif()

if(CMAKE_BUILD_TYPE STREQUAL Coverage)
include(CodeCoverage)
setup_target_for_coverage(
setup_target_for_coverage_lcov(
NAME
CLI11_coverage
EXECUTABLE
Expand Down
6 changes: 6 additions & 0 deletions tests/HelpersTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ TEST_CASE("TypeTools: tuple", "[helpers]") {
TEST_CASE("TypeTools: tuple_to_string", "[helpers]") {
std::pair<double, std::string> p1{0.999, "kWh"};
CHECK(CLI::detail::to_string(p1) == "[0.999,kWh]");

const std::tuple<std::string> t1{"kWh"};
CHECK(CLI::detail::to_string(t1) == "kWh");

const std::tuple<double> td{0.999};
CHECK(CLI::detail::to_string(td) == "0.999");
}

TEST_CASE("TypeTools: type_size", "[helpers]") {
Expand Down
19 changes: 19 additions & 0 deletions tests/TransformTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,25 @@ TEST_CASE_METHOD(TApp, "EnumCheckedTransform", "[transform]") {
CHECK_THROWS_AS(run(), CLI::ValidationError);
}

// from to-mas-kral Issue #1086
TEST_CASE_METHOD(TApp, "EnumCheckedTransformUint8", "[transform]") {
enum class FooType : std::uint8_t { A, B };
auto type = FooType::B;

const std::map<std::string, FooType> foo_map{
{"a", FooType::A},
{"b", FooType::B},
};

app.add_option("-f,--foo", type, "FooType")
->transform(CLI::CheckedTransformer(foo_map, CLI::ignore_case))
->default_val(FooType::A)
->force_callback();

run();
CHECK(type == FooType::A);
}

// from jzakrzewski Issue #330
TEST_CASE_METHOD(TApp, "EnumCheckedDefaultTransform", "[transform]") {
enum class existing : std::int16_t { abort, overwrite, remove };
Expand Down

0 comments on commit 3539bd1

Please sign in to comment.