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

Added Tuplewise operator #711

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jonasdmentia
Copy link

The Tuplewise operator extends the functionality of the Pairwise operator to N-tuples.

The overload set is generated using a .tt template and is currently set to a max size of 4-tuple, but this could be set to much larger except that the unit tests use BreakingFunc.Of<>() which only has a limited set of overloads. If anyone wants a larger overload set then this is easily extended.

Copy link
Contributor

@Orace Orace left a comment

Choose a reason for hiding this comment

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

Actually generated file are normally presents in the repository

@jonasdmentia jonasdmentia force-pushed the add-tuplewise-operator branch from af83e1f to 0ec9b4f Compare November 15, 2019 20:34
@jonasdmentia
Copy link
Author

Actually generated file are normally presents in the repository

Oh, OK. I'll roll back that commit then.

Copy link
Member

@atifaziz atifaziz left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! It's looking good for a start. I am quite short on time these days so I'll have to review this in multiple sittings (unless others can jump in and help) but I've started with an initial one for now.

@@ -78,7 +83,15 @@
</ItemGroup>

<ItemGroup>
<Service Include="{508349b6-6b84-4df5-91f0-309beebad82d}" />
Copy link
Member

Choose a reason for hiding this comment

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

Do you know what this is and do we need it?

Copy link
Author

Choose a reason for hiding this comment

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

I believe it's because I've added the T4 template to the test project. I don't believe it's 'needed' as far as building is concerned, but I think it's basically a tag used by the IDE to expose certain things for the project.

Copy link
Member

Choose a reason for hiding this comment

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

I thought that's what the service in the following line was but seems 82a7f48d-3b50-4b1e-b82e-3ada8210c358 is for tests so this one must be for T4. I was just surprised it wasn't already there since we've been using T4 since a long time.

Copy link
Contributor

Choose a reason for hiding this comment

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

since we've been using T4 since a long time.

Not in MoreLinq.Test project, until now

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that certainly explains it, @Orace!

Copy link
Member

Choose a reason for hiding this comment

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

This means we'll need tt.sh/tt.cmd to cover the test project as well now.

MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.tt Outdated Show resolved Hide resolved
MoreLinq.Test/TuplewiseTest.g.cs Outdated Show resolved Hide resolved
jonasdmentia and others added 2 commits December 7, 2019 09:53
Co-Authored-By: Atif Aziz <code@raboof.com>
… T4 template and into a separate partial class.
@jonasdmentia jonasdmentia requested a review from atifaziz December 7, 2019 00:03
@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #711 (1a4df25) into master (283c44c) will increase coverage by 0.03%.
The diff coverage is 94.82%.

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
+ Coverage   92.52%   92.56%   +0.03%     
==========================================
  Files         113      114       +1     
  Lines        3440     3482      +42     
  Branches     1024     1039      +15     
==========================================
+ Hits         3183     3223      +40     
  Misses        192      192              
- Partials       65       67       +2     
Impacted Files Coverage Δ
MoreLinq/Tuplewise.g.cs 94.73% <94.73%> (ø)
MoreLinq/Pairwise.cs 100.00% <100.00%> (+6.25%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…rator

Conflicts resolved:

- MoreLinq.Test/MoreLinq.Test.csproj
Copy link
Contributor

@viceroypenguin viceroypenguin left a comment

Choose a reason for hiding this comment

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

Is the allocation-free a big enough of an advantage to introduce a separate operator with 3 overloads? Seems like the behavior at least is already covered by Window. Keep in mind the 2-param version is effectively the same as Lag/Lead as well.

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.

4 participants