Skip to content

support group by with 2 parameters, add tolist#103

Merged
StefH merged 1 commit intozzzprojects:masterfrom
jogibear9988:patch-2
Sep 12, 2017
Merged

support group by with 2 parameters, add tolist#103
StefH merged 1 commit intozzzprojects:masterfrom
jogibear9988:patch-2

Conversation

@jogibear9988
Copy link
Copy Markdown
Contributor

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 9, 2017

Codecov Report

Merging #103 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #103      +/-   ##
=========================================
+ Coverage   84.24%   84.3%   +0.05%     
=========================================
  Files          26      26              
  Lines        3243    3255      +12     
  Branches      481     483       +2     
=========================================
+ Hits         2732    2744      +12     
  Misses        379     379              
  Partials      132     132
Impacted Files Coverage Δ
src/System.Linq.Dynamic.Core/ExpressionParser.cs 83.03% <100%> (+0.12%) ⬆️

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 e6101dc...8e71efb. Read the comment docs.

@jogibear9988
Copy link
Copy Markdown
Contributor Author

and what about this?

void LastOrDefault();
void Single();
void SingleOrDefault();
void ToList();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a test to cover this method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes in ParseLambdaComplex_2

void LastOrDefault();
void Single();
void SingleOrDefault();
void ToList();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

And a function like ToArray, can that also be defined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Think so, but I've not tested/used

Check.That(result).Equals(3);
}

[Fact]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it possible to move this test to the file QueryableTests.GroupBy.cs or does that not fit?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The other tests in this class (for example ParseLambda_1) also test Lambda with GroupBy

@StefH
Copy link
Copy Markdown
Collaborator

StefH commented Sep 11, 2017

I did add some question to this code-review.

Check.That(result.ToArray()[0]).Equals(expected[0]);
}

[Fact]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This tests ToList()

@StefH StefH merged commit 618c68c into zzzprojects:master Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants