Expose new String.Split overloads in netcoreapp1.1#12495
Expose new String.Split overloads in netcoreapp1.1#12495weshaggard merged 3 commits intodotnet:masterfrom
Conversation
| // The below extension methods allow the tests to run on targets that *do not* have the new Split overloads. | ||
| // On targets that *do* have the new Split overloads (e.g. netcoreapp1.1), the actual overloads on String | ||
| // are called instead of the extension methods below (due to the way extension methods work). | ||
| internal static class StringSplitExtensions |
There was a problem hiding this comment.
This seemed much simpler than splitting out the new overloads tests to their own String.SplitTests.netcoreapp1.1.cs file. I also considered creating a StringSplitExtensions.cs that is only included when Condition="'$(TargetGroup)' != 'netcoreapp1.1'", but that seemed overly complex as well.
I confirmed that when running the tests with /p:TargetGroup=netcoreapp1.1, the actual string.Split methods are called instead of the extension methods.
There was a problem hiding this comment.
Do we have any way to verify that at runtime? I'm afraid that we may end up in a place where we accidentally start using these in the future and not catch it. If we don't have any clever way to do that I would prefer we put these in a separate file and condition the inclusion of that file in the project.
|
cc: @weshaggard |
Conditionally included only when TargetGroup != netcoreapp1.1.
A perf test was relying on the temporary extension methods in the unit tests. Switch over to using one of the older overloads in the perf test.
| <Compile Include="Perf.Int32.cs" /> | ||
| <Compile Include="Perf.IntPtr.cs" /> | ||
| <Compile Include="Perf.StringBuilder.cs" /> | ||
| <Compile Include="..\System\String.SplitTests.cs" /> |
There was a problem hiding this comment.
Why remove these tests here, as opposed to including the extensions?
There was a problem hiding this comment.
This is the only place where a unit test file is being included in the perf tests. I think it is a mistake. The perf test was written using the temporary extensions methods (I believe as a mistake). It should have been testing one of the older Split overloads, so there's no need to use the extension methods.
| #if netcoreapp11 | ||
| public string[] Split(char separator, System.StringSplitOptions options = System.StringSplitOptions.None) { return default(string[]); } | ||
| public string[] Split(char separator, int count, System.StringSplitOptions options = System.StringSplitOptions.None) { return default(string[]); } | ||
| public string[] Split(string separator, System.StringSplitOptions options = System.StringSplitOptions.None) { return default(string[]); } |
There was a problem hiding this comment.
Ah good, so we did decide to go with @svick's optional parameter idea.
|
@weshaggard, any other feedback? |
|
Thanks @justinvp |
Expose new String.Split overloads in netcoreapp1.1 Commit migrated from dotnet/corefx@9009dc0
5 participants
Fixes #1513
Also added a test to ensure
Split(null)callsSplit(char[])without any compiler ambiguity and some minor test cleanup to reflect that actual overloads that we ended up with.