Tyrrrz / CliFx

Class-first framework for building command-line interfaces
MIT License
1.5k stars 61 forks source link

Custom value validation #42

Closed Tyrrrz closed 4 years ago

Tyrrrz commented 4 years ago

Currently, the only way to validate a value is to check it in ExecuteAsync and throw CommandExecution if it's invalid. This can be made to look nicer with tools like FluentValidation but it's still quite verbose and cumbersome.

One option is to use the standard ValidationAttribute but it has some bloat.

Tyrrrz commented 4 years ago

It seems that ValidationAttribute doesn't exist in .NET Standard 🙄

ron-myers commented 4 years ago

@Tyrrrz - do you have any future direction on this?

I have an app that requires some validation and makes me want to align with the CliFx future.

Tyrrrz commented 4 years ago

Hi @ron-myers. No direction. Pretty much all my thoughts are in this thread. I'm not sure what is the best way to implement this. Maybe attributes isn't it? If you have ideas, please share. :)

oshustov commented 4 years ago

Hello again :) what about this one? Any ideas? Maybe the validation logic can be related to the conversion logic? It looks like we shouldn't convert the value if it is not valid for the specified custom type

Tyrrrz commented 4 years ago

Hi @AlexandrShustov

Off top of my head, I have two ideas:

  1. Use ValidationAttribute (will need to add some Systems package). This way we can use existing validation attributes like [Email]/[Regex(...)], etc. The downside is that the ValidationAttribute is a rather ugly interface.

  2. Extend the existing attributes, e.g. [CommandOption(..., Validators = new[] {typeof(ValidatorA), typeof(ValidatorB)})].

Second approach sounds better to me. I don't know if standard validations are that useful. Thoughts?

In any case, we should support multiple validators (so Validators in approach 2 is an array and not a single type). And we should validate the actual value (after conversion), not the original string argument value.

oshustov commented 4 years ago

Yes, seems like the standart validation isn't that useful in our case. The second approach looks quite simple and clear, I'd like to take it.

Tyrrrz commented 4 years ago

Awesome!

WaldemarLehner commented 4 years ago

I am not experienced with Attributes, so I am not sure if this is possbile, but isn't a Validator basically a Function that returns a boolean value based on if the input is valid?

How about having an IEvaluator interface that cointains a bool Evaluate(object obj); Then you have Evaluators implementing the Interface. For example one could use the following to create an Evaluator to check if a number is within a range:

using System;

public class IComparableRangeEvaluator<T> : IEvaluator where T : IComparable<T>
{
    private readonly T lower;
    private readonly T upper;
    private readonly bool lowerInclusive;
    private readonly bool upperInclusive;

    public IComparableRangeEvaluator(T lower, T upper, bool isLowerInclusive = true, bool isUpperInclusive = true)
    {
        this.lower = lower;
        this.upper = upper;
        this.upperInclusive = isUpperInclusive;
        this.lowerInclusive = isLowerInclusive;
    }

    public bool Evaluate(object obj)
    {
        if (obj is null || !(obj is T))
        {
            return false;
        }

        T value = (T)obj;

        // Check lower bound
        if (this.lowerInclusive)
        {
            if (value.CompareTo(this.lower) < 0)
            {
                return false;
            }
        }
        else
        {
            if (value.CompareTo(this.lower) <= 0)
            {
                return false;
            }
        }

        // Check upper bound
        if (this.upperInclusive)
        {
            if (value.CompareTo(this.upper) > 0)
            {
                return false;
            }
        }
        else
        {
            if (value.CompareTo(this.upper) >= 0)
            {
                return false;
            }
        }
        return true;
    }
}
static void Main(string[] args)
        {
            IEvaluator eval = new IComparableRangeEvaluator<int>(5, 10);
            Console.WriteLine(eval.Evaluate(5));
            Console.WriteLine(eval.Evaluate(10));
            Console.WriteLine(eval.Evaluate(11));
        }

prints

True
True
False

How would you feel about such a "lower-level" approach? One could then build upon that. The projects makes use of the fluent API. How about a fluent API, that produces an IEvaluator[ ] ?

Tyrrrz commented 4 years ago

Well, a simple way to validate things right now is:

if (!IsWithinRange(SomeValue))
    throw new CommandException("Oops");

The reason for attributes is that it allows you to hide that noisy code away and reuse between commands.