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

Too strict validators #161

Open
SGSSGene opened this issue Nov 9, 2022 · 3 comments
Open

Too strict validators #161

SGSSGene opened this issue Nov 9, 2022 · 3 comments

Comments

@SGSSGene
Copy link
Contributor

SGSSGene commented Nov 9, 2022

The concept sharg::validator seems to strict:

template <typename validator_type>
concept validator = std::copyable<std::remove_cvref_t<validator_type>> &&
requires { typename std::remove_reference_t<validator_type>::option_value_type; } &&
requires(validator_type validator,
typename std::remove_reference_t<validator_type>::option_value_type value)
{
{validator(value)} -> std::same_as<void>;
{validator.get_help_page_message()} -> std::same_as<std::string>;
};

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 eseiler changed the title To strict validators Too strict validators Nov 9, 2022
@eseiler
Copy link
Member

eseiler commented Nov 9, 2022

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
Copy link
Member

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
Copy link
Member

eseiler commented Nov 11, 2022

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)};
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants