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/j value #99

Merged
merged 8 commits into from
Jul 2, 2015
Merged

Feature/j value #99

merged 8 commits into from
Jul 2, 2015

Conversation

minidfx
Copy link

@minidfx minidfx commented Jun 29, 2015

I had some problems with Nustache to identify a boolean with Newtonsoft because it return a JValue instead of a boolean as type.

I added a new micro service to identify the value of the Newtonsoft.Json.Linq.JValue with its tests.

@minidfx
Copy link
Author

minidfx commented Jun 30, 2015

Currently I have updated the Nuget package on my server as prerelease version but I prefer to use your official Nuget package.

When do you think take a look on this pull requests?

@Romanx
Copy link
Collaborator

Romanx commented Jul 1, 2015

Hey there,

It seems like it would work perfectly fine however it goes out of the ValueGetter/ValueGetterFactory structure that Nustache uses to find its values.

I'd be hesitant about including it purely as this logic should be within that structure and it could set bad precedent.

I was planning to look at performing essentially the same logic but within a factory (for checking if the object is a JToken and in a ValueGetter which performs the logic of getting the typed values from the object.

This was IsTruthy wouldn't have to be adjusted as it would be performed on primitive types.

What do you think of that solution?

@minidfx
Copy link
Author

minidfx commented Jul 1, 2015

Ok, I will apply the logic into a new one ValueGetter with its factory.

@Romanx
Copy link
Collaborator

Romanx commented Jul 1, 2015

Perfect thank you, if you need any advice feel free to ask. I would do it myself but i'm currently working on a side project and have little time.

Benjamin Burgy added 4 commits July 1, 2015 09:46
…into PropertyDescriptorValueGetterFactory class.
…alueGetterFactory class for determining when the value is a JValue and return the right value.
@minidfx
Copy link
Author

minidfx commented Jul 1, 2015

Done, I saw that you already have PropertyDescriptorValueGetterFactory and I just added a condition into PropertyDescriptorValueGetter to check whether the value received is a JValue.

The previous unit tests still works very well.

@minidfx
Copy link
Author

minidfx commented Jul 2, 2015

Hello,

When do you think check the PR and update the Nuget package? Because I need an update soon and I whether it's too late, I will create a new one just for me.

@Romanx
Copy link
Collaborator

Romanx commented Jul 2, 2015

I should be able to merge this and get it pushed today. Within the next few hours, work permitting at latest this evening GMT.

@minidfx
Copy link
Author

minidfx commented Jul 2, 2015

Thank you! Sorry to rush you ;)

@Romanx Romanx self-assigned this Jul 2, 2015
@Romanx Romanx added the feature label Jul 2, 2015
@Romanx Romanx mentioned this pull request Jul 2, 2015
@Romanx Romanx added this to the 1.15.2 milestone Jul 2, 2015
@Romanx Romanx merged commit 819ae5c into jdiamond:master Jul 2, 2015
@minidfx minidfx deleted the feature/jValue branch July 2, 2015 11:45
@Romanx
Copy link
Collaborator

Romanx commented Jul 2, 2015

This has been merged in for you and released. Thanks for your contribution.

@minidfx
Copy link
Author

minidfx commented Jul 2, 2015

Thank you for your quick answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants