StringComparer.FromComparison test#19153
Conversation
| public static System.StringComparer OrdinalIgnoreCase { get { throw null; } } | ||
| #if netcoreapp | ||
| public static System.StringComparer FromComparison(System.StringComparison comparisonType) { throw null; } | ||
| #endif |
There was a problem hiding this comment.
I don't think we add ifdefs anymore - AFAIK we build the ref only for netcoreapp, so just remove the ifdefs and it should be fine.
Here's example: #16996
Note: I may be wrong (I didn't try it myself so far :() check the configuration of the package modified in #16996, maybe there is more to it ... cc @danmosemsft @weshaggard who know more about it
Once we clarify the steps, how-to here, please update our new contrib docs on wiki Expose API in CoreFX
There was a problem hiding this comment.
Correct, we don't need an #if here as we want to expose it for all platforms.
There was a problem hiding this comment.
Makes sense, though it was requested in #14623 (comment).
Does the second part of the referenced comment still apply?
There was a problem hiding this comment.
You did the right thing by naming the file..netcoreapp.. and conditionally compiling the file. We don't use versions anymore.
|
Given that this just exposes API which is already implemented for a while, I would consider it also for 2.0 (once it is reviewed & passes tests) - @Petermarcu thoughts? In general we don't take new APIs and large non-critical contributions throughout May 10 - because we are stabilizing for .NET Core 2.0 release -- see details in https://github.com/dotnet/corefx/issues/17619#issuecomment-296872132. This may be an exception ... let's see what @Petermarcu says. |
|
And of course I forgot the most important thing: Thanks for your contribution @MikevanDongen! |
| [MemberData(nameof(FromComparison_TestData))] | ||
| public static void FromComparisonTest(StringComparison comparison, StringComparer comparer) | ||
| { | ||
| Assert.Equal(comparer, StringComparer.FromComparison(comparison)); |
There was a problem hiding this comment.
I've just tested this, which resulted in a Xunit.Sdk.SameException when using it with CurrentCulture or CurrentCultureIgnoreCase. It seems the property always returns a new instance: https://github.com/dotnet/coreclr/blob/1a32d7f2c1be31741c3e71fe35eb2fd22664d12c/src/mscorlib/shared/System/StringComparer.cs#L38
I'm not certain why it was implemented that way, but I don't think it should be tested here using Assert.Same.
We should take it. @karelz, are there other new APIs that have been implemented in coreclr but not yet exposed in corefx? We should make sure they're all exposed. |
|
@stephentoub yeah, there was a bunch of left-overs from @AlexRadch - I marked them originally as 2.0, but later triage decided to push them out. I tried to mark them at least as wishlist, but I don't have confidence I did it for all. |
|
@karelz yes just diff corelib with the rest. Il do it. |
|
@karelz @stephentoub I looked at all the public API on corelib that isn't already exposed and marked issues to expoes each with the label "api-to-expose-from-coreclr" There are also a few members on corelib that are only public for infrastructural reasons and in on case I opened a cleanup issue. |
|
Thanks for doing that, Dan. |
| <ProjectGuid>{6C314C9B-3D28-4B05-9B4C-B57A00A9B3B9}</ProjectGuid> | ||
| <AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
| <DefineConstants Condition="'$(TargetsUnix)' == 'true'" >$(DefineConstants);Unix</DefineConstants> | ||
| <DefineConstants Condition="'$(TargetGroup)'=='netcoreapp'">$(DefineConstants);netcoreapp</DefineConstants> |
There was a problem hiding this comment.
Did you actually need this define? I didn't see it used anywhere.
9 participants
Added @AlexRadch's tests for method StringComparer.FromComparison.
See issue #13800 and PR (abandoned) #14623