Skip to content

Add Jil JSON test #36

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

Closed
wants to merge 1 commit into from
Closed

Add Jil JSON test #36

wants to merge 1 commit into from

Conversation

NickCraver
Copy link

This moves the JSON.Net test to /json/newtonsoft (to match the other multi-implementation test patterns) and adds /json/jil as another JSON serialization test.

@dnfclas
Copy link

dnfclas commented Dec 9, 2015

Hi @NickCraver, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@@ -15,7 +15,8 @@
"Microsoft.AspNet.Server.WebListener": "1.0.0-*",
"Microsoft.AspNet.StaticFiles": "1.0.0-*",
"Microsoft.Extensions.WebEncoders": "1.0.0-*",
"Newtonsoft.Json": "7.0.1"
"Newtonsoft.Json": "7.0.1",
"Jil": "2.13.0-*"
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a specific version here?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah sure - I wasn't sure what the desire was since Jil's in beta for a bit while some missing corefx bits are added (waiting on https://github.com/dotnet/corefx/issues/4543 for another rev). I'll update to be specific though.

Edit: isn't this actually inconsistent with all the other libs? e.g. this is how @DamianEdwards did Dapper as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything except Newtonsoft.Json uses wildcard... just sayin'

There are pros and cons of either approach; it saved me having to issue a PR when I last rev'd "dapper" ;p

@DamianEdwards
Copy link
Member

@NickCraver would you mind rebasing this so we can measure?

This moves the JSON.Net test to /json/newtonsoft (to match the other
multi-implementation test patterns) and adds /json/jil as another JSON
serialization test.
@NickCraver
Copy link
Author

@DamianEdwards done!

@davidfowl
Copy link
Member

This PR is stale now, needs a do-over.

@NickCraver
Copy link
Author

I've paused RC2 work since things are still changing and we don't want to eat the time on the library side. I've ported Sigil (needed for Jil) but Jil still needs a fair amount of work for core. Given that many of the API differences that make it more difficult to port (and maintain performance) might be coming back...I'm personally inclined to leave it until that happens and I'm fairly sure Kevin's in the same boat.

That being said, I know Kevin would be happy to take an RC2 pull request on it. Due to RC1 tooling craziness current master doesn't support core at all (it was torn out to get a release out the door), so be aware it's pretty much a non-core to RC2 core port in the current state. I had a branch going for this, but to get Jil updated and run the performance tests I also need protobuf and we hit coreclr.exe crashing exceptions on that front. Theoretically, Jil is closer but so far away from an RC2 update on that branch it may be better to start over. Completing the set of libraries needed for testing is also non-trivial. I'm not sure if SimplerSpeedTester which Jil also uses is even planned for a port, that one also means changing all benchmark code which creates a conundrum of what comparisons are even valid. That'd have to be a cross-branch port for apples to apples ensuring no performance degradation.

TL;DR: I've done this PR twice now - don't have it in me to do it a third time. But, if someone else is willing to RC2-ify Jil then I'm happy to review it. If you want to close this out that's disappointing but may be best. It's a shame we never got a test run.

@mikeharder mikeharder closed this Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants