Support .NET Standard 2.0

[Bouke]

I recently came across this library, and it has a few features over MoreLinq that I could use: mostly the async versions. However for my use-case I'd need support for netstandard2.0, which is currently not being targeted (#153).

This PR is an attempt to add support for netstandard2.0. The build works, but there are a few test failures that I haven't looked into. I'm posting this PR mostly to get some feedback on the direction I've taken, and to see test the waters before I pour additional effort into this.

I've opted to use the NETCOREAPP constant to cover both netstandard2.0 and net48. The test projects need to target net48 to test the netstandard2.0 version of the library.

One concern is that I've marked the polyfills as public, so that Index and Range are usable. It seems to work, but might be something to evaluate. The alternative is removing all those API for the netstandard2.0 target.

[viceroypenguin]

Thanks for the PR! I appreciate the interest, and I've been wanting to have this conversation for a long time, obviously.

The biggest question I have is what to do about Index and Range. Obviously if they are public, then they will conflict with any other public definitions in other libraries that the consumer may already own. However, if we remove the Index and Range apis, then that reduces the potential offering for consumers.

Have you given any thought to the conflict that having two libraries with public Index and Range structs would raise?

[viceroypenguin]

Based on dotnet/runtime#104170, I'd like to wait until Microsoft.Bcl.Memory is published to Nuget. Then we can depend directly on this package in ns2.0 instead of relying on Polysharp. This will remove the need to have a conversation around public access to Index and Range.

[Bouke]

Thanks for the feedback!

Shipping Index / Range to consumers will not be a great experience, even if they also use PolySharp. I already had the "great" experience of resolving conflicts as both SuperLinq and SuperLinq.Async shipped these. So I see a few options going forward:

• Wait for Poly fill Index and Range types dotnet/runtime#104170 to ship: unknown time frame
• Ship PolySharp polyfills public: problematic
• Hide API using Index / Range, unhide once Poly fill Index and Range types dotnet/runtime#104170 ships

I've updated the PR to implement the last option: hide API using Index / Range. This enables going further with this PR and not be blocked by future updates to dependencies.

[viceroypenguin]

I'd rather see the Index and Range APIs excluded entirely rather than set to internal, I think?

[Bouke]

I'd rather see the Index and Range APIs excluded entirely rather than set to internal, I think?

There are many overloads forwarding the implementation to the Index and Range versions. If I were to remove them, I'd need to rewrite the implementations / perform many refactorings. See for example FindIndex:

SuperLinq/Source/SuperLinq/FindIndex.cs

Line 41 in 807dd88

return source.FindIndex(predicate, 0, int.MaxValue);

[Bouke]

I've reconstruct Unshipped from netcoreapp3.1, removing Index/Range API. Sadly Rider doesn't support applying all Roslyn fixits, and dotnet format analyzers --diagnostics=RS0016 didn't produce entries for generated code.

[Bouke]

The test projects must not target netstandard2.0 (is not something you can "run"), but net48. Suggestions on how I replace netstandard2.0 with net48 in the test project's TargetFrameworks?

<TargetFrameworks>$(TargetFrameworks.Replace("netstandard2.0","net48"))</TargetFrameworks>

This produces:

Error: /usr/share/dotnet/sdk/9.0.100-preview.6.24328.19/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(100,5): error NETSDK1046: The TargetFramework value 'net48;netcoreapp3.1;net6.0;net8.0;net9.0' is not valid. To multi-target, use the 'TargetFrameworks' property instead. [/home/runner/work/SuperLinq/SuperLinq/Tests/SuperLinq.Async.Test/SuperLinq.Async.Test.csproj::TargetFramework=net48 netcoreapp3.1 net6.0 net8.0 net9.0]

[codecov]

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.62%. Comparing base (4d7b91a) to head (e9d1899).

Files	Patch %	Lines
Source/SuperLinq.Async/Join.MergeJoin.cs	50.00%	1 Missing ⚠️
Source/SuperLinq/Join.MergeJoin.cs	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #672      +/-   ##
==========================================
- Coverage   95.64%   95.62%   -0.03%     
==========================================
  Files         247      247              
  Lines        8861     8864       +3     
  Branches     1616     1616              
==========================================
+ Hits         8475     8476       +1     
- Misses        194      196       +2     
  Partials      192      192              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[viceroypenguin]

@Bouke It builds now at least! I'm still gonna work through what I think is best regarding Index/Range though.

[viceroypenguin]

Ok, take a look and tell me what you think. With NO_INDEX as the define, it should be easy to restore these APIs later when it's possible. These are separate from the NETCOREAPP define, which will remain due to BCL differences.

[Bouke]

@Bouke It builds now at least! I'm still gonna work through what I think is best regarding Index/Range though.

Yeah, great! After removing Condition="'$(GITHUB_ACTIONS)' == 'true'" I could see the errors locally as well.

Ok, take a look and tell me what you think. With NO_INDEX as the define, it should be easy to restore these APIs later when it's possible. These are separate from the NETCOREAPP define, which will remain due to BCL differences.

Distinguishing between NO_INDEX and NETCOREAPP makes the code easier to follow, that's a win. Whether to disable the additional APIs as you did is up to you; it'll be easy to add them in the future also. So overall LGTM!

[Bouke]

Maybe you can set my expectations on when you might be creating a new release with these changes included?

[viceroypenguin]

v6.2.0 is out now! 😄