seqan / sharg-parser

The modern argument parser for c++ tools
https://docs.seqan.de/sharg
Other
10 stars 7 forks source link

Too strict validators #161

Open SGSSGene opened 1 year ago

SGSSGene commented 1 year ago

The concept sharg::validator seems to strict: https://github.com/seqan/sharg-parser/blob/203eb81912dbb40e64257a5af101fc73c08404b1/include/sharg/validators.hpp#L49-L57

It enforces that a validator has a member type option_value_type. This is the only used as a parameter for operator(). This approach doesn't properly support validators that have template operator().

This collide with validators as used in raptor positive_integer_validator, which is a generic validator for any integer.

https://github.com/seqan/raptor/blob/e14ab05c538b3847348ab144a8f2a62ac6cc794a/include/raptor/argument_parsing/validators.hpp#L71-L107

eseiler commented 1 year ago

Usually, you'd do a templatized validator

template <typename option_value_t>
    requires std::is_arithmetic_v<option_value_t>
class arithmetic_range_validator
{
    using option_value_type = option_value_t;

    void operator()(option_value_type const & cmp) const;
};

I just fixed a option_value_type in raptor, because it was easier and all my options are unsigned

smehringer commented 1 year ago

Hmm but @SGSSGene is right I think. Why do we need option_value_type in the first place? I have the feeling it is because of the possibility of chaining validators. We test if two validators can be changed by checking whether their option_value_types have a common base type.

@SGSSGene can you check? Maybe there is another way of doing this without requiring a member type variable.

eseiler commented 1 year ago

https://docs.seqan.de/sharg/main_dev/classsharg_1_1detail_1_1validator__chain__adaptor.html#details

Some snippets:

/* Note that both validators must operate on the same option_value_type in order to
 * avoid unexpected behaviour and ensure that the sharg::parser::add_option
 * call is well-formed. (add_option(val, ...., validator) requires
 * that val is of same type as validator::option_value_type).
 */

class validator_chain_adaptor
{
public:
    using option_value_type =
        std::common_type_t<typename validator1_type::option_value_type, typename validator2_type::option_value_type>;

    // [...]

    template <typename cmp_type>
        requires std::invocable<validator1_type, cmp_type const> && std::invocable<validator2_type, cmp_type const>
    void operator()(cmp_type const & cmp) const
    {
        vali1(cmp);
        vali2(cmp);
    }
};
template <validator validator1_type, validator validator2_type>
    requires std::common_with<typename std::remove_reference_t<validator1_type>::option_value_type,
                              typename std::remove_reference_t<validator2_type>::option_value_type>
auto operator|(validator1_type && vali1, validator2_type && vali2)
{
    return detail::validator_chain_adaptor{std::forward<validator1_type>(vali1), std::forward<validator2_type>(vali2)};
}