search

buvinghausen / TaskTupleAwaiter

Async helper library to allow leveraging the new ValueTuple data types in C# 7.0
MIT License
71 stars 10 forks source link

Thoughts on extending to more than 10 tuple elements? #27

Closed barnon-apiiro closed 8 months ago

barnon-apiiro commented 8 months ago

I know this sounds extreme but I'm working in a code base that reaches 13,14,15 distinct tasks in Task.WhenAll. Are you willing to increase the maximum (to 15? 20?)?

I'll happily open a PR if so.

jnm2 commented 8 months ago

I'm open to it, and definitely thanks for the offer! The BCL goes to an arity of 16 for things like Action<>. @buvinghausen What do you think?

barnon-apiiro commented 8 months ago

I'm open to it, and definitely thanks for the offer! The BCL goes to an arity of 16 for things like Action<>. @buvinghausen What do you think?

I would also add that while ValueTuple technically goes up to 8, the compiler utilizes the last item to enable larger tuples, for example: https://sharplab.io/#v2:D4AQTAjAsAUCDMACciDCiDetE+UkALIgLIAUAlJtrjSBAJymkQA0r7bnH3XvP/fQd3LkA3NRwBfWJKA=

jnm2 commented 8 months ago

Yes, in fact the only value allowed in the TRest position must itself be a tuple, as with the Tuple class also. I think an arity of 16 at the level of the C# tuple type (as opposed to considering the arity of the ValueTuple type) is more aligned with the length of a parameter list such as Action's, which is where 16 comes from.

buvinghausen commented 8 months ago

@jnm2 I would defer to you I certainly don't have the time at present to dedicate to the undertaking but should we get a PR and tests I'm happy to merge and release it.

jnm2 commented 8 months ago

@barnon-apiiro We'd be grateful for a PR with test coverage! I'd suggest an arity of 16 as our new maximum, but I'm open to discussion on that point.

jnm2 commented 8 months ago

Changing the assignment because @i3arnon has a PR up already: https://github.com/buvinghausen/TaskTupleAwaiter/pull/28

barnon-apiiro commented 8 months ago

Changing the assignment because @i3arnon has a PR up already: https://github.com/buvinghausen/TaskTupleAwaiter/pull/28

They're both me :) It's my split personality..

jnm2 commented 8 months ago

Oh, okay :D

buvinghausen commented 8 months ago

@barnon-apiiro this has been published to nuget as version 2.1.0 with your contributions added

https://www.nuget.org/packages/TaskTupleAwaiter/2.1.0

Let me know if anything else is needed

buvinghausen commented 8 months ago

@jnm2 why are you confused?

jnm2 commented 8 months ago

I'm so sorry, that was a misclick! Your work is appreciated!

buvinghausen commented 8 months ago

Ha ok just checking I was like man I know he approved the PR :)