Skip to content

Make ExpressionPromoter public + Fix issue with null constant expression compare#266

Merged
StefH merged 1 commit intozzzprojects:masterfrom
david-garcia-garcia:hotfix-null-compare
Apr 24, 2019
Merged

Make ExpressionPromoter public + Fix issue with null constant expression compare#266
StefH merged 1 commit intozzzprojects:masterfrom
david-garcia-garcia:hotfix-null-compare

Conversation

@david-garcia-garcia
Copy link
Copy Markdown
Contributor

@david-garcia-garcia david-garcia-garcia commented Apr 23, 2019

A couple of changes in this PR:

  • Checks for "null" constant expressions were done against Constants.NullLiteral. This is "wrong" mostly because nulls that did not source in that same literal will fail to compare as this is an instance level comparison. The following will evaluate to false:

Constants.NullLiteral == Expression.Constant(null)

Expression.Constant(null) == Expression.Constant(null)

To avoid confusion, I completely removed the three "Literals" in constants.

I also made ExpressionPromoter public, so that it can be extended from outside the project.

…omoter is pluggable) + Fix wrong Null compare against
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 23, 2019

Codecov Report

Merging #266 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #266   +/-   ##
======================================
  Coverage    96.5%   96.5%           
======================================
  Files          41      41           
  Lines        7213    7213           
======================================
  Hits         6961    6961           
  Misses        252     252
Impacted Files Coverage Δ
...tem.Linq.Dynamic.Core/Parser/ExpressionPromoter.cs 100% <100%> (ø) ⬆️
.../System.Linq.Dynamic.Core/Parser/KeywordsHelper.cs 100% <100%> (ø) ⬆️
...ystem.Linq.Dynamic.Core/Parser/ExpressionParser.cs 98.65% <100%> (ø) ⬆️
...ystem.Linq.Dynamic.Core/Parser/ExpressionHelper.cs 100% <100%> (ø) ⬆️
src/System.Linq.Dynamic.Core/Parser/Constants.cs 100% <100%> (ø) ⬆️

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 57d26ef...d6d0782. Read the comment docs.

@StefH StefH merged commit 4a5ed3c into zzzprojects:master Apr 24, 2019
@StefH StefH added the bug label May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

2 participants