[Mono-dev] Mono.Options test no longer builds

[Jonathan Pryor]

On May 1, 2017, at 4:09 PM, Vincent Povirk <madewokherd at gmail.com> wrote:
> I have 2 questions.
> 
> 1. Was there any good reason to change the existing constructor in the net_4_x profile?

The `Mono.Options.CommandSet` type was added on January 31, nearly 3 months ago, but that doesn’t mean it’s gotten an extensive API review. It was reviewed by it’s author…and random people I asked for API reviews (and I didn’t do a very good job of asking around).

Which is to say, just because the constructor was there doesn’t mean it was a good constructor.

That’s my rationale for considering a change in the first place.

> After the commit, we still have a constructor that provides the functionality that's missing from PCL, in the net_4_x profile. So why switch to doing it in a different way?

If we’d kept the original constructor and overloaded with the PCL-compatible overload, the net_4_x profile would have:

	partial class CommandSet {
		public CommandSet(string suite, MessageLocalizerConverter localizer = null, TextWriter output = null, TextWriter error = null);
		public CommandSet(string suite, TextWriter output, TextWriter error, MessageLocalizerConverter localizer = null);
	}

which looks drunk. The same arguments, with a different order. The only reason to do so would be to preserve source compatibility, but this API is 3 months old; how much of an existing installed base is there?

Additionally, overloading the constructor in this fashion means that there is a CS0121 ambiguity error if you pass `null` for the 2nd and 3rd parameters:

	var s = new CommandSet (“suite”, null, null);

Another option would have been to “just" remove the nullability from all the arguments and add the ArgumentNullException when output and error are null. This would require always specifying the localizer, which seems silly to me, and would also require that Console.Out/Error always be specified in desktop use.

Another option would be to provide different constructors in different profiles. This is madness; you’d lose any possibility of source compatibility between PCL and non-PCL profiles.

Finally, We could have required just the new constructor, and not added the net_4_x convenience overload:

	partial class CommandSet {
		// Remove this; who needs convenience!
		public CommandSet(string suite, MessageLocalizerConverter localizer = null);
	}

I don’t like this idea. PCL-support is a “nice-to-have.” Desktop use is *required* — the primary point of its existence is for command-line apps, which only run on a full desktop profile! — and I want to keep this as simple as possible. Requiring that people pass Console.Out or Console.Error seems silly, when — in my estimation — 99.99% of the time you’d want Console.Out/Error *anyway*.

 - Jon