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

support group by with 2 parameters, add tolist #103

Merged
merged 1 commit into from
Sep 12, 2017

Conversation

jogibear9988
Copy link
Contributor

No description provided.

@codecov
Copy link

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
Contributor Author

and what about this?

@@ -181,6 +182,7 @@ interface IEnumerableSignatures
void LastOrDefault();
void Single();
void SingleOrDefault();
void ToList();
Copy link
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
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

@@ -181,6 +182,7 @@ interface IEnumerableSignatures
void LastOrDefault();
void Single();
void SingleOrDefault();
void ToList();
Copy link
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
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

@@ -54,6 +54,48 @@ public void ParseLambda_EmptyParameterList()
}

[Fact]
public void ParseLambdaComplex_1()
Copy link
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
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
Collaborator

StefH commented Sep 11, 2017

I did add some question to this code-review.

}

[Fact]
public void ParseLambdaComplex_2()
Copy link
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