rsdn / CodeJam

Set of handy reusable .NET components that can simplify your daily work and save your time when you copy and paste your favorite helper methods and classes from one project to another
MIT License
258 stars 35 forks source link

.NET 5.0 with C# 9 #123

Closed NN--- closed 3 years ago

NN--- commented 3 years ago

Merge after #124

ig-sinicyn commented 3 years ago

@NN--- Thanks for the great work!) I'm going to look more closely at the PR tomorrow.

ig-sinicyn commented 3 years ago

As suggestion:

#pragma warning disable SYSLIB0012 // 'Assembly.CodeBase' is obsolete: 'Assembly.CodeBase and Assembly.EscapedCodeBase are only included for .NET Framework compatibility. Use Assembly.Location instead.

            var uri = new Uri(assembly.CodeBase);

#pragma warning restore SYSLIB0012

I think we should follow the recommendation and use CodeBase only when targeting to the net FW

ig-sinicyn commented 3 years ago
#if LESSTHAN_NET50
        [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
#endif

I'm not sure but this looks kinda strange for me. The line was ok for all prev targets, what changed with FW 5.0 ?

ig-sinicyn commented 3 years ago

As for me, everything else is ok.

One more time, a great thanks you! :)

NN--- commented 3 years ago

@ig-sinicyn Next part is to

#if LESSTHAN_NET50
      [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
#endif

I'm not sure but this looks kinda strange for me. The line was ok for all prev targets, what changed with FW 5.0 ?

.NET 5.0 added ObsoleteAttribute to this :) Before .NET 5.0 it didn't have any meaning in .NET Core, but was not obsolete. https://github.com/dotnet/runtime/commit/329f0dbfa3a164ee2e3bf9f75b7641165995c799#diff-997b403ad145cdd790196073af16aa4df8b1fc688415ce38b1c8a788e438dd2a

ig-sinicyn commented 3 years ago

.NET 5.0 added ObsoleteAttribute to this :)

Wow, nice catch!

NN--- commented 3 years ago

@ig-sinicyn AppVeyor still don't have VS 16.8 , hence the preview version. I think it is stable enough for CodeJam. Once they update their image, we can switch back to release.

NN--- commented 3 years ago

Fixed comments. Good enough to commit ?:) I started adding nullability, but it requires C# 9.0 in order to work correctly with generic methods.

ig-sinicyn commented 3 years ago

Fixed comments. Good enough to commit ?:)

Yep, LGTM 👍

NN--- commented 3 years ago

Thanks. Then waiting for appveyor. It takes some time to compile :)

NN--- commented 3 years ago

Do you know why the build is so slow ? 42 min 8 sec

ig-sinicyn commented 3 years ago

Have no idea

NN--- commented 3 years ago

Why the build failed ?

NN--- commented 3 years ago

nunit3-console doesn't support net50 yet.

NN--- commented 3 years ago

@ig-sinicyn Thanks for approval. I see we encountered the new .NET 5.0 breaking change with strings.:( Need to investigate and fix before merging.

NN--- commented 3 years ago

Here is what fails. Need to figure out how to make a better test. https://github.com/dotnet/dotnet-docker/issues/1877#issuecomment-622125813

NN--- commented 3 years ago

Found the relevant documentation: https://docs.microsoft.com/en-us/dotnet/standard/base-types/string-comparison-net-5-plus

NN--- commented 3 years ago

@ig-sinicyn any idea how to test invariant culture ? I used ss and ß characters but they are now not the same in 5.0

ig-sinicyn commented 3 years ago

@ig-sinicyn any idea how to test invariant culture ? I used ss and ß characters but they are now not the same in 5.0

Feel free to disable tests (with some comment, of course). I'm going to scan all codebase for the CA1307: Specify StringComparison for clarity CA1309: Use ordinal StringComparison after PR completion.

UP. #125

NN--- commented 3 years ago

I made test which compares ss with ß , validating invariant comparison is used. But in .NET 5.0 they are not the same even with invariant comparison. The question do we need to find some other invariant equal character or just remove this case ?:)

ig-sinicyn commented 3 years ago

The question do we need to find some other invariant equal character or just remove this case ?:)

Just remove

NN--- commented 3 years ago

Found the problem. AppVeyor has .NET 5.0 preview which defined NETCOREAPP3_1 for .NET 5.0 :( Added a workaround.