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

Implement ToImmutable*Async extension methods #1545

Merged
merged 11 commits into from
Aug 28, 2024

Conversation

bash
Copy link
Contributor

@bash bash commented May 24, 2021

Resolves #1540.

I copied most of the XML documentation and tests of the non-immutable equivalents (with adjustments of course).

b667720 contains a bit of a refactoring, but I'm not sure if it's a big improvement.

Working with AsyncOverloads.tt was a bit unpleasant, especially since the generated code has diverged from the generated code (nullability and XML documentation) which meant I had to be careful of what to commit and what to discard.
Wouldn't a source generator be a better fit, because it knows about nullability and generic constraints?

@dnfadmin
Copy link

dnfadmin commented May 24, 2021

CLA assistant check
All CLA requirements met.

@dnfadmin
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ bash sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@bartdesmet bartdesmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo a cosmetic nit around a namespace.

@bartdesmet
Copy link
Collaborator

A source generator could be a viable option to replace the T4. We've had these T4 files for years but as you've observed sometimes manual changes to the .cs files made their way in and slipped through the cracks in code reviews, so we should either do a pass to refresh them, or replace them. In the latter case, we need to make we end up generating the exact same code of course.

@bartdesmet
Copy link
Collaborator

@bash I've merged the PR for the source generators (thanks!), so could you go ahead and rebase this PR (due to the delete/edit conflicts for the T4 files which are now obsolete)?

@bash
Copy link
Contributor Author

bash commented Aug 25, 2021

@bartdesmet Hurray! I'm really happy how the source generators turned out. This PR is all rebased and ready for another review. 😃

using System.Threading.Tasks;
using static System.Linq.FunctionalHelpers;

namespace System.Linq
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally, everything looks good. But I do have a concern around the symmetry with the .NET libraries which put all of these ToImmutable* extension methods in System.Collections.Immutable instead (e.g. see here). It'd be unexpected if a user currently has using System.Collections.Immutable; (but no using System.Linq;) and is unable to discover the Async variants of these ToImmutable* methods.

My suggestion is to move all of these to System.Collections.Immutable and organize them in classes named as Immutable[<collection type here>]AsyncEnumerableExtensions, so the discoverability story is consistent.

A potential follow-up question is whether we want to put all of these in the current assembly or keep them in a separate assembly, but that always stirs heated debates (there's a history of flip-flopping between coarse-grained and fine-grained organization). So I'm fine keeping it here, and just focus on the logical namespace organization. The physical organization in terms of assemblies could be scrutinized down the road, but it seems like the conditional dependency on System.Collections.Immutable is only conditional for 1 target platform, so it's okay to always have the reference when the BCL assembly is available in the box anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yess I finally got around to address your comment 🥳 (and fix my source generator ^^)

Oh, I haven't thought about this! I applied your suggestion for the namespace change and class name change. I also moved the files to match the new namespace.

There's not much that I know about how to best divide up assemblies, but it seems ok to me too to depend on System.Collections.Immutable conditionally, if it's only for one target platform.

@Romfos
Copy link

Romfos commented Jul 2, 2022

one year later -_-

@bash bash requested a review from bartdesmet July 28, 2022 12:20
@stijnherreman
Copy link

one year later -_-

Looks like there are no active maintainers currently :(

@akarnokd
Copy link
Collaborator

Does this have to live in dotnet/reactive? C# has extension methods so unlike other Rx incarnations, there is no loss of convenience having it in a 3rd party library.

@Romfos
Copy link

Romfos commented Nov 16, 2022

Does this have to live in dotnet/reactive? C# has extension methods so unlike other Rx incarnations, there is no loss of convenience having it in a 3rd party library.

I think yes, .NET 6 have this method out of the box for linq

@bash
Copy link
Contributor Author

bash commented Nov 16, 2022

I always assumed that the goal of System.Linq.Async is to provide everything for async enumerables that Linq provides for enumerables (Although technically the ToImmutable* extension methods are part of System.Collections.Immutable).

I'd be willing to move this to a separate project if this should not be part of System.Linq.Async and someone has a good name suggestion for the package :)

@bash
Copy link
Contributor Author

bash commented Aug 27, 2024

I'm no longer interested in updating this PR should a maintainer against all odds choose to review it.
Anyone is free to adopt the PR though in that case :)

@idg10
Copy link
Collaborator

idg10 commented Aug 28, 2024

/AzurePipelines run

Copy link

Pull request contains merge conflicts.

@idg10 idg10 changed the base branch from main to feature/bash-to-async-immutable August 28, 2024 13:34
@idg10
Copy link
Collaborator

idg10 commented Aug 28, 2024

/AzurePipelines run

Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@idg10 idg10 changed the base branch from feature/bash-to-async-immutable to main August 28, 2024 13:41
@idg10 idg10 changed the base branch from main to feature/bash-to-async-immutable August 28, 2024 13:41
@idg10
Copy link
Collaborator

idg10 commented Aug 28, 2024

Hi,

Sorry this got neglected for so long. My employer (endjin.com) took over maintenance of all of Rx at the start of last year. Our priority was around Rx itself, and the fact that we inherited the Ix and System.Linq.Async projects as well was just a consequence of the history of this project. The BCL team has made noises about moving the System.Linq.Async functionality into .NET itself, with a strong suggestion that they might need to chuck a lot of code away because it doesn't really conform to their current design guidelines. So we've not been putting much attention into this.

However, since that seems to have stalled, we're starting to look at this again.

Since all the changes were made 3 years ago, this PR is no longer really usable, so what I'm intending to do is merge it into a separate branch, and then I will merge main into that branch, and then try to bring all of this up to date, with the hope of finally getting it merged into main.

@idg10 idg10 merged commit 5a1ea0a into dotnet:feature/bash-to-async-immutable Aug 28, 2024
3 checks passed
@bash bash deleted the to-async-immutable branch August 28, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ToImmutable*Async extension methods to System.Linq.Async
8 participants