Initial work on the JSON5.stringify method #35
Conversation
|
Hey @aeisenberg, sorry for the delay, but I'm finally getting to the backlog of JSON5 issues. I'll look at this really soon now, promise. =) Thanks again for submitting. |
|
Thanks. Let me know if there's anything you need. |
|
Hi, wondering if you can get a chance to look at this PR. I'm using json5 in another project now and it would be nice to use a released version. Thanks. |
|
Oh man, I'm sorry @aeisenberg. I'm clearly not able to be an effective maintainer right now, so I'd like to try something that I read about ( (cc @MaxNanasy) I'd love for JSON5 to remain a cohesive project, which often means minimal and simple, so I do ask that you please still leave "editorial oversight" to me, e.g. let's discuss new features before you implement them. =) In this particular case, if you want to introduce this, there's a few more things that should happen here:
What do you think? |
|
Forgot to mention: nice work on implementing this, and thanks for adding all the tests! |
|
Also, to finally address your original comments:
No problem. I think it's fine to have an incomplete implementation, as long as it's clearly documented. (E.g. this JSON5 implementation itself doesn't support Unicode characters in key names yet.) So let's keep that in mind and document both the readme and changelog appropriately.
It has some utility, and I've seen it used in a few places. E.g. https://gist.github.com/mikeal/2504336 Like the above, that doesn't have to be implemented today, but we should still plan on it I think. The good news is it shouldn't be too hard to (eventually) implement: it's ultimately just calling a function on every key-value pair. =)
The community would be the best source of feedback. Unfortunately at the moment we don't have a channel to easily reach the community. I had started a Google Group way back when, but never shared the link: https://groups.google.com/group/json5 For now, I'll at least add a comment to the issue requesting this feature, where several people +1'ed, but let's look to make the Google Group a thing going forward. I'll add it to the readme! Nice work again. =) |
|
Sorry for the multiple-comment spam; another thing: I haven't looked deeply at your implementation, and it looks simple/elegant enough to me, but FYI it might help to look at Douglas Crockford's original implementation as well (if you haven't already) for a reference check: https://github.com/douglascrockford/JSON-js/blob/master/json2.js#L228 (Not that Doug's original implementation is the best model implementation =D, but JSON5's parse implementation is based directly on his, FWIW. Though there's been talk of changing that…) |
|
Thanks for the commit rights. As you mention, there's still some work to do here. I think the guthub hack might actually work in this case. :) I'll address the documentation and other issues over the next few days. |
basic support all available tests included space argument partially implemented, but not tested, so currently disabled (throws exception on use) replacer argument is not implemented (throws exception on use)
* added tests for spaces argument * testing many more corner cases * fixed lots of problems
Initial work on the JSON5.stringify method. All complete. Fully tested and documented. Will raise issues for remaining work.
2 participants
Here is my work on the stringify method. Basic work is complete and tested. The space argument (3rd) is partially implemented, but disabled until I can fully test. The replacer argument (2nd) is not implemented. I don't plan to implement this one since it seems a bit esoteric to me.
I probably have a few more hours worth of work before I can tidy up the space argument support. But, feedback is welcome up until this point. Thanks.