Skip to content

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

Merged
StefH merged 6 commits intozzzprojects:masterfrom
jogibear9988:master
Jun 1, 2017
Merged

[Fix] Fixed null in Parameter and added functionality Binary And and Or with different Types#86
StefH merged 6 commits intozzzprojects:masterfrom
jogibear9988:master

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