Implement System.ComponentModel.VersionConverter

[0xced]

VersionConverter is a new System.ComponentModel.TypeConverter subclass that handle conversions between string and System.Version.

Edit @stephentoub 6/12/2018: https://github.com/dotnet/corefx/issues/28594

[karelz]

@0xced thanks for your PR. You are adding new public API, which needs to go through API review first.
Can you please file an issue with API description, motivation, etc. and work with area owners (@maryamariyan @safern) to agree on that first? Thanks!

[0xced]

I just rebased on master and I see that some tests are still failing.

• On NETFX x86 Release Build, UWP CoreCLR x64 Debug Build and UWP NETNative x86 Release Build:
  The builds fail with this error, I don't understand why.

VersionConverterTests.cs(11,24): error CS0246: The type or namespace name 'VersionConverter' could not be found (are you missing a using directive or an assembly reference?) [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.ComponentModel.TypeConverter\tests\System.ComponentModel.TypeConverter.Tests.csproj]

• On Linux x64 Release Build and OSX x64 Debug Build:
  The builds succeed but some tests are failing in System.Net.HttpListener.Tests and System.Net.NameResolution.Functional.Tests which seem totally unrelated to this pull request.

• On my machine (macOS 10.12.6) I get this error when I try to build the System.ComponentModel.TypeConverter project (It worked fine around 350 commits ago, before I rebased on master today):

$ ./build.sh src/System.ComponentModel.TypeConverter
Tools are already initialized
Running: ~/Projects/corefx/Tools/msbuild.sh /nologo /verbosity:minimal /clp:Summary /maxcpucount /l:BinClashLogger,Tools/Microsoft.DotNet.Build.Tasks.dll;LogFile=binclash.log /p:ConfigurationGroup=Debug  /flp:v=normal  /flp2:warningsonly;logfile=msbuild.wrn  /flp3:errorsonly;logfile=msbuild.err  src/dirs.proj /p:DirectoryToBuild=~/Projects/corefx/src/System.ComponentModel.TypeConverter 
  System.Runtime -> ~/Projects/corefx/bin/ref/System.Runtime/4.2.1.0/netcoreapp/System.Runtime.dll
~/Projects/corefx/Tools/FrameworkTargeting.targets(346,5): error MSB4018: The "FindBestConfigurations" task failed unexpectedly. [~/Projects/corefx/src/System.Runtime/ref/System.Runtime.csproj]
~/Projects/corefx/Tools/FrameworkTargeting.targets(346,5): error MSB4018: System.ArgumentException: No value was provided for property 'TargetGroup' and no default value exists [~/Projects/corefx/src/System.Runtime/ref/System.Runtime.csproj]
~/Projects/corefx/Tools/FrameworkTargeting.targets(346,5): error MSB4018:    at Microsoft.DotNet.Build.Tasks.ConfigurationFactory.ParseConfiguration(String configurationString, Boolean permitUnknownValues) [~/Projects/corefx/src/System.Runtime/ref/System.Runtime.csproj]
~/Projects/corefx/Tools/FrameworkTargeting.targets(346,5): error MSB4018:    at Microsoft.DotNet.Build.Tasks.FindBestConfigurations.Execute() [~/Projects/corefx/src/System.Runtime/ref/System.Runtime.csproj]
~/Projects/corefx/Tools/FrameworkTargeting.targets(346,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() in E:\A\_work\45\s\src\Build\BackEnd\TaskExecutionHost\TaskExecutionHost.cs:line 631 [~/Projects/corefx/src/System.Runtime/ref/System.Runtime.csproj]
~/Projects/corefx/Tools/FrameworkTargeting.targets(346,5): error MSB4018:    at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__25.MoveNext() in E:\A\_work\45\s\src\Build\BackEnd\Components\RequestBuilder\TaskBuilder.cs:line 787 [~/Projects/corefx/src/System.Runtime/ref/System.Runtime.csproj]

Build FAILED.

I’m a bit lost here, I’m not sure what I should do to fix these issues.

[danmoseley]

@karelz @wfurt for System.Net.HttpListener.Tests and System.Net.NameResolution.Functional.Tests can we move them out of inner loop? We don't want any flaky tests in CI.

[danmoseley]

For the other issues I would do git clean -fdx then try again.

To be clear though this API needs approval from the API review board. They might not convene for a couple weeks, and might reject it or want changes...

[danmoseley]

@0xced the API was approved, can you make any necessary changes to match the approved shape, and get tests green?

@dotnet-bot test this please

[0xced]

I keep rebasing on master but NETFX x86 Release, UWP CoreCLR x64 Debug and UWP NETNative x86 Release keep failing while all other platforms are fine.

Does anybody has an idea what’s happening with those three platforms?

[stephentoub]

Does anybody has an idea what’s happening with those three platforms?

The new type you've added only exists in .NET Core; it doesn't exist in .NET Framework or UWP. So the tests when built for .NET Framework or UWP are failing to compile. You need to ensure that your new tests are only built when targeting .NET Core, e.g. with a condition in the project file like Condition="'$(TargetGroup)' == 'netcoreapp'".

[0xced]

I just rebased on master and 🎉 all the tests are finally green. ✅

[safern]

LGTM, thanks @0xced

[karelz]

@safern why did you mark it as NO MERGE?
This is fairly old PR, how can we push it forward?

[safern]

I added NO MERGE because I'm waiting for response on my comment above: #28516 (comment)

[danmoseley]

@0xced could you respond to @safern feedback? seem that is all that is left.

[0xced]

I'm currently away from computer, I will reply in a few weeks.

…

On 23 Jul 2018, at 20:03, Dan Moseley ***@***.***> wrote: @0xced could you respond to @safern feedback? seem that is all that is left. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[danmoseley]

@0xced sure no problem. I'll let this PR sit then.

[karelz]

@0xced any update? If you can't work on it now, let's close it and reopen when you have time again. Thanks!

[safern]

LGTM.

[safern]

Thanks @0xced

[0xced]

Do you know when it will land in .NET Core? I just checked in the recently released version 2.2 and System.ComponentModel.VersionConverter does not exist.

[karelz]

@0xced it is in 3.0 milestone. Try 3.0 Preview1 - it went out today as well.

[0xced]

Excellent! I didn't see it was in a milestone, thanks.

[hughbe]

Couple of things that I would like to point out that are inconsistent between UriTypeConverter/VersionConverter and the other type converters in .NET Core:

• They throw ArgumentNullException for CanConvertFrom(null) whereas .NET Framework returns false. I suggest fixing this to be consistent with other type converters.
• They return false for CanConvertFrom(typeof(InstanceDescriptor)) because they don't call their base class CanConvertFrom method. See Adhere to desktop implementation of [Can]ConvertFrom #35760. I suggest fixing this
• They don't support InstanceDescriptor conversion - should this be added to VersionConverter?

[niemyjski]

Sure would be nice if every type listed here had a comment on the class itself or in docs someplace that stated it had a type converter. The first place I looked when serialization broke for me on the upgrade to 3.1 from 2.2 was on the Version class and I didn't see it. I searched for usages on GitHub and it's parser must have problems with [typeof(Version)] as I searched for typeof(Version) and typeof(System.Version) with no luck. JamesNK/Newtonsoft.Json#2243