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

[Fix] Fixed null in Parameter and added functionality Binary And and Or with different Types #86

Merged
merged 6 commits into from
Jun 1, 2017

Conversation

jogibear9988
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented May 31, 2017

Codecov Report

Merging #86 into master will increase coverage by 0.11%.
The diff coverage is 69.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
+ Coverage   84.07%   84.19%   +0.11%     
==========================================
  Files          25       25              
  Lines        3090     3131      +41     
  Branches      449      471      +22     
==========================================
+ Hits         2598     2636      +38     
+ Misses        364      357       -7     
- Partials      128      138      +10
Impacted Files Coverage Δ
src/System.Linq.Dynamic.Core/ExpressionParser.cs 81.48% <69.04%> (+0.3%) ⬆️

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 35bdf2c...ad1e3d5. Read the comment docs.

@jogibear9988
Copy link
Contributor Author

Maybe this could also be included in the nuget ;-) ?

@StefH StefH changed the title Fix null in Parameter [Fix] null in Parameter Jun 1, 2017
@StefH StefH changed the title [Fix] null in Parameter [Fix] Fixed null in Parameter and added functionality Binary And and Or with different Types Jun 1, 2017
@jogibear9988
Copy link
Contributor Author

Can this be merged?

@StefH
Copy link
Collaborator

StefH commented Jun 1, 2017

I provided some code-review comments, can you please fix ?

@jogibear9988
Copy link
Contributor Author

Where? I don't see them!

}

[Fact]
public void ExpressionTests_Test_BinaryAndOr()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some remarks:

  1. Does this method also test OR ? Because I only see & ?
  2. Rename this method to ExpressionTests_BinaryAnd and also ExpressionTests_BinaryOr ?
  3. Move this method to the correct location (I try to keep a A-Z sorting applied to this test class to keep things organized)
  4. Also check the result (result0, result1, ...) using Assert, else the unit-test is not complete.

@StefH
Copy link
Collaborator

StefH commented Jun 1, 2017

Review is requested now, sorry I did not press the button... ;-)

Copy link
Contributor Author

@jogibear9988 jogibear9988 left a comment

Choose a reason for hiding this comment

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

done

@jogibear9988
Copy link
Contributor Author

if it's okay, can you merge and create a nuget?

@StefH StefH merged commit 8c9daf0 into zzzprojects:master Jun 1, 2017
@StefH
Copy link
Collaborator

StefH commented Jun 1, 2017

1.0.7.2 / 1.0.4.2 added to NuGet

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