Skip to content

Combine up to 5 providers #78316

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

StephaneDelcroix
Copy link

while combining more than 2 providers, the returning tuple is very confusing, chaining Left.Left.Left etc.

this adds more combine overloads that returns a flat tuple

while combining more than 2 providers, the returning tuple is very confusing, chaining Left.Left.Left etc.

this adds more compine overloads that returns a flat tuple
@StephaneDelcroix StephaneDelcroix requested a review from a team as a code owner April 25, 2025 11:14
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 25, 2025
@dotnet-policy-service dotnet-policy-service bot added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 25, 2025
@StephaneDelcroix
Copy link
Author

I'm using those as extensions methods

Copy link
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

This is an API change and needs to go through API review.

@CyrusNajmabadi
Copy link
Member

@StephaneDelcroix Can you write up an API suggestion on this? I'd be willing to try to shepherd it through (no promises of course). I do find teh x.Left.Right.Left goop to be annoying myself. So having a small set of helpers here seems reasonable.

@333fred
Copy link
Member

333fred commented Apr 25, 2025

@@ -29,8 +29,20 @@ public static class IncrementalValueProviderExtensions
public static IncrementalValueProvider<ImmutableArray<TSource>> Collect<TSource>(this IncrementalValuesProvider<TSource> source) => new IncrementalValueProvider<ImmutableArray<TSource>>(new BatchNode<TSource>(source.Node), source.CatchAnalyzerExceptions);

public static IncrementalValuesProvider<(TLeft Left, TRight Right)> Combine<TLeft, TRight>(this IncrementalValuesProvider<TLeft> provider1, IncrementalValueProvider<TRight> provider2) => new IncrementalValuesProvider<(TLeft, TRight)>(new CombineNode<TLeft, TRight>(provider1.Node, provider2.Node), provider1.CatchAnalyzerExceptions);

public static IncrementalValuesProvider<(TProvider1 provider1, TProvider2 provider2, TProvider3 provider3)> Combine<TProvider1, TProvider2, TProvider3>(this IncrementalValuesProvider<TProvider1> provider1, IncrementalValueProvider<TProvider2> provider2, IncrementalValueProvider<TProvider3> provider3) => provider1.Combine(provider2).Combine(provider3).Select(static (t, _) => (t.Left.Left, t.Left.Right, t.Right));
Copy link
Member

Choose a reason for hiding this comment

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

note: i'm not sure why it's TProviderN instead of like TValueNorTSourceN. Another option is to simply not name the tuple elements as they do not seem to provide much value. In other words, i'm not sure how saying t.provider1is better than justt.Item1`. So consider just making this:

public static IncrementalValuesProvider<(TSource1, TSource2, TSource3)> Combine<TSource1, TSource2, TSource3>(this IncrementalValuesProvider<TSource1> provider1, IncrementalValueProvider<TSource2> provider2, IncrementalValueProvider<TSource3> provider3) => provider1.Combine(provider2).Combine(provider3).Select(static (t, _) => (t.Left.Left, t.Left.Right, t.Right));

Note: i would do this in the API you post. We'll also look over and have a review in the near future to hammer out names/shapes, and will post the results back.

@333fred 333fred marked this pull request as draft April 25, 2025 19:03
@333fred
Copy link
Member

333fred commented Apr 25, 2025

Moving to draft until there is an API review session for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants