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

Add tests from JSONTestSuite #49

Merged
merged 2 commits into from Oct 6, 2018
Merged

Add tests from JSONTestSuite #49

merged 2 commits into from Oct 6, 2018

Conversation

Kaiepi
Copy link
Contributor

@Kaiepi Kaiepi commented Oct 5, 2018

Fixes #47

@timo
Copy link
Owner

timo commented Oct 5, 2018

One of the new lines apparently makes github (and git itself) believe that the test file is binary now; perhaps you can find out which it is and use some escaping to make it plain-old text again?

Also i'd appreciate a comment in the arrays that tells where the tests from this source start, and maybe also a link to the original.

Other than that, good work, thanks!

@Kaiepi
Copy link
Contributor Author

Kaiepi commented Oct 5, 2018

The tests that were making the file appear to be binary were the ones that used unescaped UTF16 and U+FFFF. I removed those, so now it's back to being a text file.

I'll update the pullreq once Rakudo finishes building and I get a chance to test again
Edit: updated

@timo
Copy link
Owner

timo commented Oct 5, 2018

Sorry about the nitpicking, but could you split replacing << >> with « » into a separate commit so that they show up "properly" as different changes in a "git blame"?

@Kaiepi
Copy link
Contributor Author

Kaiepi commented Oct 6, 2018

Sure

@timo
Copy link
Owner

timo commented Oct 6, 2018

fantastic, thanks!

@timo timo merged commit ac2bf17 into timo:master Oct 6, 2018
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.

None yet

2 participants