[Mono-dev] Program Option Parsing Library

[Jonathan Pryor]

On Fri, 2008-01-11 at 10:25 -0800, Jay Logue wrote:
> Jonathan Pryor wrote: 
> > Now how should localization be handled?  Should it?
>
> It only seems fair.  I would do the localization in the
> OptionException class itself.  I see other mono Exception types
> calling Locale.GetText() to translate their text (e.g.
> FileNotFoundException), but I don't see where its declared.  (I see
> one in mcs/class/Managed.Windows.Forms/Assembly/Locale.cs but I don't
> think that's it).

Yes, but...  Currently the except message text itself is (potentially)
generated across two different places; consider Options.Add<T>:

        catch (Exception e) {
          throw new OptionException (
            string.Format (
              "Could not convert string `{0}' to type {1} for options `{{0}}'.",
              s, typeof(T).Name), 
            options, e);
        }

Options.Add<T>() has one string.Format() call, while OptionException has
another (to insert the actual option string into the final string).

One thing to do would be to alter OptionException to take a
`OptionException(string format, string[] arguments)' constructor, so
that instead of building up the final string piece-meal, it does
everything.

Alternatively, OptionException could become *dumber* than it is (i.e.
remove all string formatting), and have Options do ALL formatting.  This
has the advantage that an Options constructor overload could take a
Func<string,string[],string> delegate instance which would be
responsible for ALL translation ((f,a) => string.Format(f,a)).

Thinking about it, this seems the saner solution.

The one downside is that Func`3 is specific to .NET 3.5, while (aside
from syntax and tests) Options _should_ currently be .NET 2.0 compatible
(though I haven't tested this).  Consequently Func`3 should be avoided;
I guess I should create a new delegate type:

	delegate string OptionLocalizer (string format, string[] args);

Other possible type names appreciated (and I don't like Rafael's
TranslateIt name. ;-)

> A couple of other thoughts: 
> 
> -- I think it would be convenient to the user if the error message
> included the option value whenever ConvertFromString() failed.

Already done, as seen via the catch block above, and in
Test.CheckExceptions:

        AssertException (typeof(OptionException),
          "Could not convert string `value' to type Int32 for options `n='.",
          p, v => { v.Parse (_("-n", "value")); });

The one downside is that the actual option used which triggered the
exception is lost.  For example, if you did:

        var p = new Options () {
          { "a|b|c|d=", (int n) => { /* ... */ } },
        };
        p.Parse (new string[]{"-b", "42", "-a", "value"});

In this case -b is valid, but -a is not.  However, the error message
wouldn't show that -a was the problem, but would instead say:

  Could not convert string `value' to type Int32 for options `a|b|c|d='.

Which is less than friendly.  Sadly, the only solution I can think of is
to use an Action<string,string> where the 1st argument would be the
actual option string used, which seems like overkill (and would also
prevent .NET 2.0 compatibility).  I'm currently willing to live with the
current output.

>   That way if there are many instances of the same option the user can
> easily figure out which one is causing the problem.  An alternative is
> to include the index of the offending argument as a property of the
> OptionException, leaving it to the caller's exception handler to point
> out the value.

This is also a good idea, and suffers the same problem as providing the
actual option used: the callback doesn't have that information, and thus
can't provide it in the OptionException.

I can think of only one workaround for both these issues that doesn't
involve changing the Action<string> delegate with something else:
provide additional context information in the Options class itself,
which Options.Add<T>() can make use of.

I don't like this idea as currently, post initialization, Options is
thread-safe (as there is no mutable data -- the Items IList<Option> from
Collection<Option> and the Dictionary<string,Option> mapping are never
modified except on addition/removal, which are both explicitly
programmer controlled).  Consequently you could create one Options
instance and, as long as all threads only call Options.Parse(), it's
completely thread safe.

I'd rather not lose this capability.

> -- I was looking at your FooConverter example and it occurred to me
> that the code might be more concise if Options.Add<T>() had an
> overload that took a TypeConverter.  That way I could simply say:
> 
>          Options p = new Options () {
>             { "v", v => ++verbose },
>             { "foo", new FooConverter(), (int v) => foo = v },
>             { "h|?|help", v => ShowHelp () }
>          };
> 
> ... rather than having to use a wrapper type (Foo) as a stand-in for
> the real type.

Foo isn't a stand-in for the real type, it *is* the real type.  Just as
`int' is the real type, not System.ComponentModel.Int32Converter (the
actual type that does the string->int conversion).

So no, I see this as a disadvantage.

 - Jon