Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modern framework support #749

Closed

Conversation

viceroypenguin
Copy link
Contributor

@viceroypenguin viceroypenguin commented Jul 9, 2020

Properly implement solution for hiding extension methods on different frameworks by removing the this modifier.

  • Supports binary compatibility by still compiling duplicated methods. Since extension methods are implemented via syntactic sugar, as long as the methods still exist, the this modifier can be removed and callers will still be able to reach the method.
  • Added more build frameworks and test frameworks in order to handle different times new methods were added.

Fixes #738; improvement over #739

@viceroypenguin
Copy link
Contributor Author

viceroypenguin commented Sep 23, 2020

@atifaziz @Orace I have released morelinq.temp as a proof of concept based on this branch. I have found this to improve compile-time compatibility while retaining run-time compatibility. This should be easier than using the using static method, that is still viable for all users.

I have checked .NET 5, and there are no changes to System.Linq.Enumerable, so maintenance is not affected by the upcoming release.

Please let me know if there are any issues with this solution.

@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #749 (b1dc561) into master (2abc6a9) will increase coverage by 0.79%.
The diff coverage is 100.00%.

❗ Current head b1dc561 differs from pull request most recent head df3aeb9. Consider uploading reports for the commit df3aeb9 to get more accurate results

@@            Coverage Diff             @@
##           master     #749      +/-   ##
==========================================
+ Coverage   87.90%   88.70%   +0.79%     
==========================================
  Files         108      108              
  Lines        3473     3470       -3     
  Branches      984      978       -6     
==========================================
+ Hits         3053     3078      +25     
+ Misses        272      251      -21     
+ Partials      148      141       -7     
Impacted Files Coverage Δ
MoreLinq/Prepend.cs 100.00% <ø> (ø)
MoreLinq/SkipLast.cs 90.90% <ø> (ø)
MoreLinq/TakeLast.cs 100.00% <ø> (ø)
MoreLinq/Append.cs 100.00% <100.00%> (ø)
MoreLinq/ToHashSet.cs 85.71% <100.00%> (ø)
MoreLinq/Permutations.cs 93.75% <0.00%> (-1.49%) ⬇️
MoreLinq/MaxBy.cs 91.81% <0.00%> (+10.90%) ⬆️
MoreLinq/Slice.cs 76.92% <0.00%> (+23.98%) ⬆️
MoreLinq/MoreEnumerable.cs 85.00% <0.00%> (+30.00%) ⬆️
MoreLinq/ListLike.cs 78.57% <0.00%> (+42.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee23824...df3aeb9. Read the comment docs.

…upport

# Conflicts:
#	MoreLinq.Test/MoreLinq.Test.csproj
#	MoreLinq/MoreLinq.csproj
@fsateler
Copy link
Member

fsateler commented Jan 9, 2021

I wonder if, instead of polluting the code with #if NET_blabla, it would be possible to apply a transform using Fody or any modern roslyn-based equivalent?

@viceroypenguin
Copy link
Contributor Author

I could do it with a custom Fody weaver, but I would want to know that you guys are comfortable adding a dependency on Fody. That said, I think this is probably simpler than doing that, IMHO. A custom weaver would require an additional project and some work to identify the appropriate methods and strip the this attribute from the parameter.

…upport

# Conflicts:
#	MoreLinq.Test/MoreLinq.Test.csproj
#	bld/ExtensionsGenerator/Program.cs
@Arithmomaniac
Copy link
Contributor

Awaiting this eagerly. Anything holding it up?

@viceroypenguin
Copy link
Contributor Author

@atifaziz I've updated with build tests to future proof this. It will become more important with .net 6, which is adding several functions that will overlap with MoreLinq (dotnet/runtime#27687).

@viceroypenguin
Copy link
Contributor Author

@atifaziz - Updated to include .net6 support. Breaking change in .MaxBy() and .MinBy() due to conflict in behavior with the new matching methods in the fx. Binary compatibility is maintained, but source is not. Please advise if you will be reviewing this or not.

@viceroypenguin
Copy link
Contributor Author

I have released a preview version of a fork SuperLinq, which I will keep updated with newer .NET versions.

@Arithmomaniac
Copy link
Contributor

@viceroypenguin You should probably unfork it once you have no intention of merging into MoreLinq anymore.

For starters, I'd want to file some issues 😉

@viceroypenguin
Copy link
Contributor Author

If you want to help by filing some issues, please do! My initial focus was just on cleaning things up and putting a preview on nuget. I'd like to start copying some issues over and then start addressing them.

@viceroypenguin
Copy link
Contributor Author

@Arithmomaniac thanks for pointing that out - I turned the issues setting on. I like your issue, so please file it. Already thinking about how to start implementing long-term.

@viceroypenguin viceroypenguin deleted the multi-framework-support branch July 5, 2022 13:35
@viceroypenguin viceroypenguin restored the multi-framework-support branch July 5, 2022 13:36
@viceroypenguin viceroypenguin reopened this Jul 5, 2022
@viceroypenguin
Copy link
Contributor Author

Closing per comments in #565 and fork released via SuperLinq

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToHashSet has been superseded in .NET Core 2.0+, .NET Framework 4.7.1+
3 participants