Add Value to eventually replace ValueObject

[atifaziz]

This PR is a follow-up from PR #100 and supersedes it. It's less provocative 😉 in the sense that it makes the same improvements without breaking any compatibility. The public API remains the same since ValueObject, unlike in PR #100, isn't being removed anymore. Instead, this PR changes & replaces all internal uses of ValueObject with the new value type Value. The arguments for this are as follows (and remain pretty much the same as for PR #100):

• ValueObject is a class and causes an additional allocation per value. Converting it to a struct will require quite some review without much added benefits except saving on the wrapper allocation.
• Add member of ValueObject was used in just one place and could be in-lined at the single call site (during leaf pattern matching); the latter was chosen. Add was the only reason ValueObject was read-write.
• Equality members like Equals and GetHashCode were never used or needed.
• It doesn't make the wrapped value any more strong-typed than using the bare object; instead it ValueObject does conversions in some cases that can fail, which isn't the case with Value.
• A ValueObject reference that can be null or an instance with a null for Value adds confusing semantics at best.
• Modern C# features like pattern matching are equally effective at succinctly encoding decisions based on type checks.

All tests still pass.

The new Value type is a read-only struct so it doesn't cause any allocations to wrap the underlying type. It's a discriminated union of the values it permits:

enum ValueKind { None, Boolean, Integer, String, StringList }

StringList is also a new read-only type that holds one or more strings laid out like a cons list. It's stronger-typed than an ArrayList that ValueObject holds, but it's immutable nature and internal implementation layout also means that it causes a cell allocation per entry. While it's a list, its native and most efficient operations make it act more like a stack. In other words, you can only prepend (push/cons) items, remove the head item (pop/cdr) or request the item at the head (peek/car). StringList also supports value equality by comparing its items for equality with those of another StringList.

While command/argument/option values are internally maintained as Value instances now, they are converted to ValueObject at the very end before returning to the user code. This is done through a new overload of Apply that enables, what I'd like to call, bring your own types (BYOT). That is, the new overload doesn't care about or enforce any representation for values as ValueObject or options as IDictionary<string, ValueObject>. The overload has the following generic signature:

internal T Apply<T>(string doc, Tokens tokens,
                    IApplicationResultAccumulator<T> accumulator,
                    bool help = true, object version = null,
                    bool optionsFirst = false, bool exit = false)

There is a new abstraction in there called IApplicationResultAccumulator<> that's defined as:

interface IApplicationResultAccumulator<T>
{
    T New();
    T Command(T state, string name, bool value);
    T Command(T state, string name, int value);
    T Argument(T state, string name);
    T Argument(T state, string name, string value);
    T Argument(T state, string name, StringList value);
    T Option(T state, string name);
    T Option(T state, string name, bool value);
    T Option(T state, string name, string value);
    T Option(T state, string name, int value);
    T Option(T state, string name, StringList value);
    T Error(DocoptBaseException exception);
}

The parsed options and their values are fed through an implementation of the above accumulator interface that then accumulates/folds to some state type T. I have added an implementation (see ValueObjectDictionaryAccumulator) that accumulates commands, arguments and options as keyed ValueObject instances to IDictionary<string, ValueObject>, thus yielding full backwards compatibility. A second implementation (ValueDictionaryAccumulator) does that same but encodes values as Value instead of ValueObject. This second implementation is only used in tests in this PR, but the main motivation for its addition is for PR #77 (we can eventually think about exposing it publicly too). While it was not necessary, the tests were updated to use Value rather than ValueObject since the bulk cases were just initializing values rather than using any features from ValueObject and the code reads simpler now.

[atifaziz]

@smoothdeveloper Sorry it took so long to follow-up on your comments in PR #100, but my availability to work on this project fell considerably over the last 3 months. Nevertheless, I took some more time to understand ValueObject and all the valid instances of its use with commands, arguments and options. I believe that this PR addresses most of the concerns you raised as well as ideas you floated.

ValueObject offers a bit of a statically typed API helping to reason about those things, while adhoc pattern matching makes it harder to reason about IMO.

The new Value type is a lot closer to ValueObject and fixes many of its problems that bothered from an allocation, conversions and correctness perspective.

@atifaziz question: if you had Discriminated Unions available in C#, would you go with naked object rather than defining the finite set of possible cases this type ends up having at runtime currently?

Value is now a discriminated union. There is just no way to express them succinctly and in a closed fashion in C# today. Records come close, but they are still classes and cause allocations. I believe value-records may be landing soon in C#, but I believe Struct Discriminated Unions like in F# are still probably a few releases away.

…, I think a discriminated union, even hand desugared in C# solely, would still be better route for the long term maintenance than just object, no matter how expressive the pattern matching constructs are nowadays.

Agree and have thus have taken the approach to introduce Value in a more proper way. If I were to be totally honest, I think I was being somewhat lazy because I knew going the whole distance with a proper DU type would require a lot of time and effort and I was eager to return to work on PR #77 thinking that these internal changes could always be improved on later.

[iwillspeak]

I really like the direction that this is going. The accumulator interface looks super powerful. I've
knocked together an example of using it to aggregate into a class instance rather than a
dictionary over here: iwillspeak@117cbe2

[iwillspeak]

Do we want this to be public? I can't see how a user would inject an accumulator otherwise.

[atifaziz]

Yes, I think eventually. For now, I wanted to bring this as an internal design, refactoring and changes that can eventually be added to the public API.

[atifaziz]

I really like the direction that this is going.

@iwillspeak Glad to hear and thanks for your thorough review!

The accumulator interface looks super powerful.

Right, the idea was to put the user code in control and detach Apply from caring at all about the output or mandating it. Eventually, I think we can generalise it even further with a generator interface and layer the accumulator interface on top of it. I believe the generator version will be even simpler and more open/powerful. I might propose it as a separate PR.

I've
knocked together an example of using it to aggregate into a class instance rather than a
dictionary over here: iwillspeak@117cbe2

Cool and really awesome to see another use case in action! I iterated another version where I removed some reflection, especially for setting the value. Instead, I went with quoted expressions, where one supplies a getter expression and an assignment/setter is dynamically inferred and compiled. One can then either provide the name-property mapping explicitly:

ApplicationResultAccumulators.Create<TestArguments>(
    ("command"                    , args => args.Command                   ),
    ("<argument>"                 , args => args.ArgArgument               ),
    ("--option"                   , args => args.FlagOption                ),
    ("--max-degree-of-parallelism", args => args.FlagMaxDegreeOfParallelism));

or just list the property and have the kebabised name inferred from the property name based on prefix rules (Arg for arguments, Flag for option and assume a command otherwise, which is what you had):

ApplicationResultAccumulators.Create<TestArguments>(
    args => args.Command,
    args => args.ArgArgument,
    args => args.FlagOption,
    args => args.FlagMaxDegreeOfParallelism);

[iwillspeak]

Yeah, all of those have their plus points. It seems like a nice abstraction.