Support for custom value parsers

[atruskie]

In the vein of automating as much as possible, it would be nice to be able to supply our own value parsers to the library. This is useful when an application has its own data structures because it removes boilerplate from argument parsing.

For example, we are parsing DateTimeOffsets, TimeSpans, Color structs, and some other domain-specific types.

If you think this would be a valuable contribution I'd be happy to implement it, but I'd appreciate a suggestion on how I should extend the public API. A trivial (but perhaps naive) way to do it would be to add an AddParser method ValueParserProvider.

[natemcmaster]

Yes, this would be a great contribution. I only made it internal because I hadn't thought through the way I want to expose this to users. One of the problems I was hoping to address before making the API public was how to make it more obvious to implements this, and which to implement, ICollectionParser, ITupleValueParser, IValueParser, or something else. Have any ideas how these could be reconciled?

[atruskie]

Sorry to be unhelpful but I'm not sure I follow. I'll ramble on and hope I approach what you were thinking.

For my purposes at least I had no need to implement custom collection parsers so I hadn't given it much thought. I had about 4 value parsers I implemented with the hack on my fork.

Other than a Dictionary<T, U> I can't imagine any other collection style that needs implementation.

The tuple parser is interesting but is currently limited (if I'm reading the code right, limited to only Tuple<bool, T>), so I see why you'd want to add extensibility. However, you seem to only have supported tuples as if they were Option<T> s. My gut feeling is that Tuples are just single values and the normal IValueParser interface could have been modified to support them.

If I were to design this from scratch, I'd have a single interface (which is just your tuple value parser) that looks like:

    /// <summary>
    /// Parses a value.
    /// </summary>
    internal interface ITupleValueParser
    {
        object Parse(bool hasValue, string argName, string value);
    }

It could parse all values, including pseudo-option types, or even full-fledged Tuple<T,U,V,W> whatevers.

The other two concerns are type registration and CommandOptionType mapping. Ultimately the API will take one of two forms:

Option A

var application = new CommandLineApplication<MainArgs>();

application.ValueParserProvider.AddParser(CommandOptionType.SingleValue, typeof(DateTimeOffset), new DateTimeOffsetParser());
application.ValueParserProvider.AddParser(CommandOptionType.SingleValue, typeof(TimeSpan), new TimeSpanParser());
// and so on
var result = await application.ExecuteAsync(args);

Option B

If we expand the value parser interface a little, we can reduce code clutter:

    /// <summary>
    /// Parses a value.
    /// </summary>
    internal interface ITupleValueParser
    {
        Type TargetType {get ; }
        CommandOptionType DefaultOptionType {get ; }
        object Parse(bool hasValue, string argName, string value);
    }

var application = new CommandLineApplication<MainArgs>();

application.ValueParserProvider.AddParser(new DateTimeOffsetParser());
application.ValueParserProvider.AddParser(new TimeSpanParser());
// and so on
var result = await application.ExecuteAsync(args);

---

Finally, one last concern would be whether we could replace existing value providers. While the need is purely hypothetical (which is reason enough not to support it) it may be useful to replace one the of the default value parsers with your own implementation (for example, forcing a culture-invariant parsing of values).

[natemcmaster]

I thought about this more too. I agree with you about users rarely needing to implement a collection parser. Since a collection parser is typically a generic, we could have ValueParserProvider support open generic types. Internally, ITupleValueParser and ICollectionParser can wrap an instance of IValueParser. So, yes, I like the idea of a single interface.

I'm leaning towards what you proposed in Option B an interface with a method and target type, but also towards making the Parse method very simple.

public interface IValueParser
{
    Type TargetType { get; }

    /// <summary>
    /// Parses the raw value
    /// </summary>
    /// <param name="value">Raw value</param>
    /// <returns>Parsed object</returns>
    /// <throws name="System.FormatException">When the value cannot be parsed</throws>
    object Parse(string value);
}

The only reason the current implementation passes in argName was to format the exception message. I don't think we actually need that. The caller can catch exceptions and put context -- such as CommandArgument/CommandOption -- into an error message like this: Error parsing value for '{argName}': could not convert '{value}' into type '{typeName}'. {exception.Message}

one last concern would be whether we could replace existing value providers. While the need is purely hypothetical (which is reason enough not to support it) it may be useful to replace one the of the default value parsers with your own implementation

Seems easy enough to support. I'd be okay with that.

[atruskie]

I have mixed feelings about removing the argName parameter. Yes, it's boilerplate and could be handled generically, but I (somewhat emotionally :-)) resist the removing of useful information from APIs. IMO it is useful enough just for debugging.

You didn't comment on the CommandOptionType information: Maybe it would just be best to assume CommandOptionType.SingleValue for any custom value parser added?

I think this enough design for me to start a PR.

public interface IValueParser
{
    Type TargetType { get; }

    /// <summary>
    /// Parses the raw value
    /// </summary>
    /// <param name="value">Raw value</param>
    /// <returns>Parsed object</returns>
    /// <throws name="System.FormatException">When the value cannot be parsed</throws>
    object Parse(/*string argName,*/ string value);
}

Add these methods to ValueParserProvider:

class ValueParserProvider
   public void Add(IValueParser parser) { ... }
   public void AddOrReplace(IValueParser parser) { ... }
}

Exposed by the ValueParsers read-only property on CommandLineApplication

[natemcmaster]

That's a good point about providing more context to the parser. One more idea...what about this? We add an interface to both CommandArgument and CommandOption named ICommandParameter. I was thinking of doing this anyways since the types are very similar.

interface ICommandParameter
{
   string Name { get; }
   List<string> Values { get; }
   bool HasValue();
}

public interface IParameterValueConverter
{
    Type TargetType { get; }

    /// <summary>
    /// Converts parameter raw values into a type
    /// </summary>
    /// <param name="parameter"></param>
    /// <returns>Parsed object</returns>
    /// <throws name="System.FormatException">When the value cannot be parsed</throws>
    object Convert(ICommandParameter parameter);
}

[atruskie]

That's a neat idea. It would (could?) handle any other collection type as well.

[natemcmaster]

I changed my mind. I started playing with this a little more. I like your proposal better. Is this something you have time to contribute? I'm still open to a PR.

public interface IValueParser
{
    Type TargetType { get; }

    /// <summary>
    /// Parses the raw value
    /// </summary>
    /// <param name="value">Raw value</param>
    /// <returns>Parsed object</returns>
    /// <throws name="System.FormatException">When the value cannot be parsed</throws>
    object Parse(string argName, string value);
}
class ValueParserProvider
   public void Add(IValueParser parser) { ... }
   public void AddOrReplace(IValueParser parser) { ... }
}

[atruskie]

I've got some time scheduled to do a PR tomorrow (I'm busy until then)

[natemcmaster]

No rush. Thanks in advance :)