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

[Feature] Adds support for decimal qualifiers. Resolves #91 #92

Merged
merged 3 commits into from
Jun 27, 2017
Merged

[Feature] Adds support for decimal qualifiers. Resolves #91 #92

merged 3 commits into from
Jun 27, 2017

Conversation

pferraris
Copy link
Contributor

I also added a test for real numbers and checked that all tests pass successfully.

@codecov
Copy link

codecov bot commented Jun 25, 2017

Codecov Report

Merging #92 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #92      +/-   ##
=========================================
+ Coverage   84.31%   84.4%   +0.09%     
=========================================
  Files          26      26              
  Lines        3150    3162      +12     
  Branches      469     473       +4     
=========================================
+ Hits         2656    2669      +13     
  Misses        362     362              
+ Partials      132     131       -1
Impacted Files Coverage Δ
...c/System.Linq.Dynamic.Core/Tokenizer/TextParser.cs 98.61% <100%> (ø) ⬆️
src/System.Linq.Dynamic.Core/ExpressionParser.cs 82.85% <100%> (+0.18%) ⬆️

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 cbb3900...8974db3. Read the comment docs.

Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

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

  • Code looks good.
  • Please order the test alphabetically in the file.

@@ -193,6 +193,23 @@ public void ParseLambda_ParameterName_Null()
}

[Fact]
public void ParseLambda_RealNumbers()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please order the test alphabetically in the file.

@@ -657,6 +657,38 @@ public void ExpressionTests_FloatQualifiers_Negative()
}

[Fact]
public void ExpressionTests_DecimalQualifiers()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please order the test alphabetically in the file.

}

[Fact]
public void ExpressionTests_DecimalQualifiers_Negative()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please order the test alphabetically in the file.

@@ -132,6 +132,30 @@ public void TextParser_Parse_RealLiteralPlus()
}

[Fact]
public void TextParser_Parse_RealLiteralFloatQualifier()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please order the test alphabetically in the file.

}

[Fact]
public void TextParser_Parse_RealLiteralDecimalQualifier()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please order the test alphabetically in the file.

@StefH
Copy link
Collaborator

StefH commented Jun 25, 2017

Thanks.

Can you take a look at the review?

@pferraris
Copy link
Contributor Author

It's done! I already ordered the tests, except for ParseLambda_IllegalMethodCall_ThrowsException which is below all the others.

@StefH StefH merged commit ab249d4 into zzzprojects:master Jun 27, 2017
@StefH StefH changed the title Adds support for decimal qualifiers. Resolves #91 [Feature] Adds support for decimal qualifiers. Resolves #91 Jun 27, 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