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

New features #117

Merged
merged 4 commits into from
Nov 28, 2017
Merged

New features #117

merged 4 commits into from
Nov 28, 2017

Conversation

jogibear9988
Copy link
Contributor

@jogibear9988 jogibear9988 commented Nov 9, 2017

  • Configuration for Expression Parsing
  • new for named Types

@codecov
Copy link

codecov bot commented Nov 9, 2017

Codecov Report

Merging #117 into master will decrease coverage by 0.62%.
The diff coverage is 43.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   83.66%   83.03%   -0.63%     
==========================================
  Files          26       27       +1     
  Lines        3275     3331      +56     
  Branches      484      489       +5     
==========================================
+ Hits         2740     2766      +26     
- Misses        395      433      +38     
+ Partials      140      132       -8
Impacted Files Coverage Δ
src/System.Linq.Dynamic.Core/GlobalConfig.cs 95% <ø> (+5%) ⬆️
src/System.Linq.Dynamic.Core/ParsingConfig.cs 0% <0%> (ø)
...em.Linq.Dynamic.Core/DynamicQueryableExtensions.cs 98.26% <100%> (ø) ⬆️
...ystem.Linq.Dynamic.Core/DynamicExpressionParser.cs 56.52% <27.27%> (-43.48%) ⬇️
src/System.Linq.Dynamic.Core/ExpressionParser.cs 81.59% <51.28%> (-0.42%) ⬇️
...c/System.Linq.Dynamic.Core/Tokenizer/TextParser.cs 98.63% <0%> (+1.36%) ⬆️

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 28d726e...aebdd1b. Read the comment docs.

@jogibear9988 jogibear9988 changed the title New feature -> Configuration for Expression Parsing New features -> Configuration for Expression Parsing, new for named Types Nov 9, 2017
@jogibear9988 jogibear9988 changed the title New features -> Configuration for Expression Parsing, new for named Types New features Nov 9, 2017
@jogibear9988
Copy link
Contributor Author

Is this patch mergeable?

@StefH
Copy link
Collaborator

StefH commented Nov 14, 2017

Whats the difference between this and https://github.com/StefH/System.Linq.Dynamic.Core/blob/master/src/System.Linq.Dynamic.Core/GlobalConfig.cs?

@jogibear9988
Copy link
Contributor Author

That GlobalConfig is static! If I use dynamiclinq within different assemblies, I can not have a different configuration!

@jogibear9988
Copy link
Contributor Author

@StefH any comments to this commit?

@StefH
Copy link
Collaborator

StefH commented Nov 17, 2017

I did add a small comment in GetTypeInfo.

And I still need to review more code in this patch.

@jogibear9988
Copy link
Contributor Author

do I need to fix smth?

@StefH
Copy link
Collaborator

StefH commented Nov 24, 2017

smth?

@jogibear9988
Copy link
Contributor Author

something?

@StefH
Copy link
Collaborator

StefH commented Nov 24, 2017

You can fix my comment on type.GetTypeInfo().BaseType

@jogibear9988
Copy link
Contributor Author

I do not find it!

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.

See comment

propertyInfos = propertyInfos.Where(x => x.Name != "Item");
}
#elif NETSTANDARD
if (type.GetTypeInfo().BaseType == typeof(DynamicClass))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can use type.GetTypeInfo().BaseType for all frameworks, so no need for an if-elif structure.

@StefH
Copy link
Collaborator

StefH commented Nov 24, 2017

Sorry. Please check again.

@jogibear9988
Copy link
Contributor Author

jogibear9988 commented Nov 24, 2017

@jogibear9988
Copy link
Contributor Author

And you still compile for net 4.0 so it is needed

@codecov-io
Copy link

codecov-io commented Nov 27, 2017

Codecov Report

Merging #117 into master will decrease coverage by 0.62%.
The diff coverage is 46.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   83.66%   83.04%   -0.63%     
==========================================
  Files          26       27       +1     
  Lines        3275     3332      +57     
  Branches      484      490       +6     
==========================================
+ Hits         2740     2767      +27     
- Misses        395      432      +37     
+ Partials      140      133       -7
Impacted Files Coverage Δ
src/System.Linq.Dynamic.Core/GlobalConfig.cs 95% <ø> (+5%) ⬆️
src/System.Linq.Dynamic.Core/ParsingConfig.cs 0% <0%> (ø)
...em.Linq.Dynamic.Core/DynamicQueryableExtensions.cs 98.26% <100%> (ø) ⬆️
...ystem.Linq.Dynamic.Core/DynamicExpressionParser.cs 56.52% <27.27%> (-43.48%) ⬇️
src/System.Linq.Dynamic.Core/ExpressionParser.cs 81.6% <57.89%> (-0.41%) ⬇️
...c/System.Linq.Dynamic.Core/Tokenizer/TextParser.cs 98.63% <0%> (+1.36%) ⬆️

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 28d726e...84461ef. Read the comment docs.

@StefH StefH merged commit 10bb5b5 into zzzprojects:master Nov 28, 2017
@jogibear9988
Copy link
Contributor Author

could you create a new nuget?

@StefH
Copy link
Collaborator

StefH commented Nov 29, 2017

I found an error :

private readonly ParsingConfig _parsingConfig;

_parsingConfig is never assigned a value

I'm fixing this now.

@StefH
Copy link
Collaborator

StefH commented Nov 29, 2017

Uploaded new 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.

3 participants