Quantcast

Mono.Options test no longer builds

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Mono.Options test no longer builds

Vincent Povirk-2
The Mono.Options tests no longer compile for the net_4_x profile after
this commit: https://github.com/mono/mono/commit/7e2571ed334e9cee3f0d3bafeef02852310f4d3b

This is because the test depends on the constructor that was removed.
I don't think the test should be changed because it correctly
indicates that the API was broken.

I have 2 questions.

1. Was there any good reason to change the existing constructor in the
net_4_x profile?

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?

2. Why didn't continuous integration catch this?

The test isn't even failing, it's not building at all. This shouldn't
be difficult for an automated system to notice.
_______________________________________________
Mono-devel-list mailing list
[hidden email]
http://lists.dot.net/mailman/listinfo/mono-devel-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Mono.Options test no longer builds

Miguel de Icaza via Mono-devel-list
Matthew/Jon, could you please comment on the API break?

Regarding the tests: it seems we don't run the Mono.Options tests in CI... Probably a bad oversight that I'll fix as soon as the tests are building again, thanks for the heads up!

- Alex

> On 1 May 2017, at 22:09, Vincent Povirk <[hidden email]> wrote:
>
> The Mono.Options tests no longer compile for the net_4_x profile after
> this commit: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmono%2Fmono%2Fcommit%2F7e2571ed334e9cee3f0d3bafeef02852310f4d3b&data=02%7C01%7Calkpli%40microsoft.com%7C08cdb7d2feb34f1d053308d490ce02b2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636292661872592866&sdata=Y01VpKgfUGWxQHpyWDvk1xYagYNY%2BKId3Plebl7%2FuVk%3D&reserved=0
>
> This is because the test depends on the constructor that was removed.
> I don't think the test should be changed because it correctly
> indicates that the API was broken.
>
> I have 2 questions.
>
> 1. Was there any good reason to change the existing constructor in the
> net_4_x profile?
>
> 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?
>
> 2. Why didn't continuous integration catch this?
>
> The test isn't even failing, it's not building at all. This shouldn't
> be difficult for an automated system to notice.
> _______________________________________________
> Mono-devel-list mailing list
> [hidden email]
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.dot.net%2Fmailman%2Flistinfo%2Fmono-devel-list&data=02%7C01%7Calkpli%40microsoft.com%7C08cdb7d2feb34f1d053308d490ce02b2%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636292661872592866&sdata=0KRC%2FnuwGfeffn5YmKRuOH%2FHEBZB876cpYHwRG9dnPY%3D&reserved=0

_______________________________________________
Mono-devel-list mailing list
[hidden email]
http://lists.dot.net/mailman/listinfo/mono-devel-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Mono.Options test no longer builds

Vincent Povirk-2
Would it make sense to build even the tests you don't run?

On Mon, May 1, 2017 at 6:09 PM, Alexander Köplinger
<[hidden email]> wrote:
>
> Regarding the tests: it seems we don't run the Mono.Options tests in CI... Probably a bad oversight that I'll fix as soon as the tests are building again, thanks for the heads up!
_______________________________________________
Mono-devel-list mailing list
[hidden email]
http://lists.dot.net/mailman/listinfo/mono-devel-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Mono.Options test no longer builds

Miguel de Icaza via Mono-devel-list
Sure. I think the only tests that we have but don't compile/run are the standalone tests in the System.Web area.

> On 2 May 2017, at 01:33, Vincent Povirk <[hidden email]> wrote:
>
> Would it make sense to build even the tests you don't run?
>
> On Mon, May 1, 2017 at 6:09 PM, Alexander Köplinger
> <[hidden email]> wrote:
>>
>> Regarding the tests: it seems we don't run the Mono.Options tests in CI... Probably a bad oversight that I'll fix as soon as the tests are building again, thanks for the heads up!

_______________________________________________
Mono-devel-list mailing list
[hidden email]
http://lists.dot.net/mailman/listinfo/mono-devel-list
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Mono.Options test no longer builds

Jonathan Pryor
In reply to this post by Vincent Povirk-2
On May 1, 2017, at 4:09 PM, Vincent Povirk <[hidden email]> 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

_______________________________________________
Mono-devel-list mailing list
[hidden email]
http://lists.dot.net/mailman/listinfo/mono-devel-list
Loading...