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.dump #9

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Json.dump #9

wants to merge 12 commits into from

Conversation

dolik-rce
Copy link

Hi,
I needed the JSON.dump method for my project and since it is not implemented yet I had to write it myself 😆

Usage is the same as described in the documentation and based on the already existing stub. It supports setting three basic formats for the output (via new JSON.style method) and custom formatting if required. Some basic tests are included as well.

Please review the code and let me know if it is suitable for merging. If you don't see any problem with it, I can also update the documentation, man page etc.

@ingydotnet
Copy link
Owner

Hi. Wow. This looks wonderful!

I'm going to review it now, and hopefully release it.

@ingydotnet
Copy link
Owner

I fixed some bugs in OSX and a little other stuff. Pushed to branch issue/9.

I like it overall. Good job!

Can you update the doc in doc/json.swim? The ReadMe and man pages etc are
generated from that doc. I can do that part.

I noticed some bugs using json-format with tab or newline. Haven't looked into
them.

printf '{"foo":2,"bar\t":"far\n"}\n' | ./bin/json-format

@dolik-rce
Copy link
Author

Ok, I'll have a look at the buggy behavior and update the docs...

@dolik-rce
Copy link
Author

I noticed some bugs using json-format with tab or newline. Haven't looked into them.

printf '{"foo":2,"bar\t":"far\n"}\n' | ./bin/json-format 

That is a separate issue in JSON.load:

(. lib/json.bash; printf '{"foo":2,"bar\t":"far\n"}\n' | JSON.load )

So it is not caused by my changes, but I'll probably look at it later...

I fixed the sorting that you broke and added a test for it 😉 I hope it will work on OS X this time. Can you please check?

The documentation is updated, including the man page and ReadMe.pod. Let me know if I overlooked something.

@dolik-rce
Copy link
Author

I noticed some bugs using json-format with tab or newline. Haven't looked into them.

printf '{"foo":2,"bar\t":"far\n"}\n' | ./bin/json-format 

That is a separate issue in JSON.load:

(. lib/json.bash; printf '{"foo":2,"bar\t":"far\n"}\n' | JSON.load )

So it is not caused by my changes, but I'll probably look at it later...

Also, tab is not allowed to be in either key nor value. I checked both ECMA-404 and RFC 7159 and according to those standards, control characters U+0000 to U+001F must be escaped. So this is not actually a bug, it is invalid input.

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.

2 participants