NightOwl888 / ICU4N

International Components for Unicode for .NET
Apache License 2.0
25 stars 8 forks source link

Add .NET Framework 4.0 support #25

Closed introfog closed 4 years ago

introfog commented 4 years ago

Hello, I'm from iText Software , Alexey Subach recently contacted you. I have implemented support for .NETFramework 4.0, please review these changes.

To build a project, you need a dependency on J2N from my other pull request.

NightOwl888 commented 4 years ago

J2N v2.0.0-beta-0007 has been released to NuGet with .NET Framework 4.0 support. You may now re-target to that NuGet package in order to complete this PR.

NightOwl888 commented 4 years ago

I have pushed some updates to the master branch, which will help out a little here:

  1. The J2N PackageReference was upgraded to 2.0.0-beta-0007.
  2. ICU4N was updated to utilize the ParsePosition class from J2N. ParsePosition was removed from ICU4N.
  3. Build features were moved to the default Default.Build.targets file.
introfog commented 4 years ago

Can you please investigate why testing falls? I also replaced all the #if NET40 with the FEATURE constants, please review.

NightOwl888 commented 4 years ago

IntegerExtensionsTest.TestAsFlagsToEnum is failing due to lack of ArgumentOutOfRangeException being thrown when it should be. See: https://dev.azure.com/ICU4N/ICU4N/_build/results?buildId=33&view=ms.vss-test-web.build-test-results-tab&runId=986&resultId=100016&paneView=debug

Please see the changes that were added to J2N to run the tests. In ICU4N, we might not need to do all of this because we had an issue with xUnit not supporting .NET Framework 4.0, but in ICU4N we don't use xUnit.

We use the TestTargetFramework.props file to determine which tests to run (and debug) within Visual Studio 2017/2019 or using dotnet test on the command line. We just uncomment one framework at a time for testing and debugging. For ICU4N we can add a target framework net40 here and here. Then you can run all tests on .NET Framework 4.0 to check for any problems.

This particular problem doesn't seem to be specific to target framework, but it also is not happening on the master branch.

introfog commented 4 years ago

Please see why the build began to fall, my changes are simple and should not affect the build.

NightOwl888 commented 4 years ago

The build failure is due to the reference to System.Collections.Immutable in the ICU4N.Tests.Collation project, which does not support .NET Framework 4.0.

Apparently, that reference is not required at all, so you may remove it to fix the issue.

introfog commented 4 years ago

An ImmutableSet was used in one place, I replaced it with a regular HashSet, is that okay?

NightOwl888 commented 4 years ago

Yes, but please consolidate it to a single line. Collections.singleton(new ULocale(malformed)); in Java translates best into the collection initializer in C# with a single parameter.

ISet<ULocale> supported = new HashSet<ULocale> { new ULocale(malformed) };

The use of System.Collections.Immutable in this case was an incorrect translation from Java.

introfog commented 4 years ago

Thanks for the quick answers and help!

Snipx commented 4 years ago

Thank you very much @NightOwl888, your responsiveness and helpfulness has been outstanding!