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

Json serialization for Unicode #972

Merged
merged 3 commits into from
Dec 8, 2015

Conversation

naveensrinivasan
Copy link

This will fix the Unicode Json serialization bug #967.

When I was looking at the code I realized there was already a fix with a PR facebook-csharp-sdk/simple-json#66 .Reused the same code for this fix.

Naveen added 3 commits December 4, 2015 14:20
Fixes for json serialization issue when unicode is present.
Tests for Unicode character serialization
Compared if the serialized data has what was expected. Not just
deserialized data.
@ferventcoder
Copy link

Sweet. :)

@haacked
Copy link
Contributor

haacked commented Dec 7, 2015

I'm hesitant to take this because SimpleJson.cs is a dependency we pull in from NuGet. Have you tried the latest version to see if it already has a fix?

If not, could you submit a PR upstream to https://github.com/facebook-csharp-sdk/simple-json?

I worry that the next time we upgrade, we'll break this. At least the unit test will catch it though. 😄

@naveensrinivasan
Copy link
Author

@haacked I looked for that first before making the change. https://github.com/facebook-csharp-sdk/simple-json hasn't been updated in the last 20 months. There is already a PR with this fix for the main repo which is still pending from April. That's the reason for making a change in this repo.

I understand your concerns. What do you want to do?

@khellang
Copy link
Contributor

khellang commented Dec 8, 2015

Ping @prabirshrestha and ask what he's been up to? 😉

@haacked
Copy link
Contributor

haacked commented Dec 8, 2015

Ok, as long as the PR is upstream, I'll take this one.

haacked added a commit that referenced this pull request Dec 8, 2015
@haacked haacked merged commit 7c81e25 into octokit:master Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants