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

A nicer CSON.stringify #9

Merged
merged 2 commits into from
Dec 3, 2014
Merged

A nicer CSON.stringify #9

merged 2 commits into from
Dec 3, 2014

Conversation

johan
Copy link
Contributor

@johan johan commented Nov 29, 2014

CSON.stringify

  • have CSON.stringify produce safe CSON that looks nicer than native JSON
  • add minimal json2cson tool that does the same, given valid JSON or CSON
  • add minimal cson2json tool to reproduce JSON, given valid JSON or CSON

Still handles all the same features as JSON.stringify does, the same way, including weird inputs.

The main difference between this cson output, and what the cson package's json2cson spits out (also what prompted this pull request) are the object literals: this codec always uses curly braces, thus making:

[
  {
    hello: 'there!'
  }
]

instead of:

[
  hello: 'there!'
]

The latter is less safe, explicit, and convenient, for instance when you have some version controlled cson array which starts out with just one huge object in it. Given the cson-safe serialization, when you later add a new object to the array, there won't be a complete reindentation of the first object. And in the case of a human editing it by hand, the mistake of making the two objects actually become just one merged object is less likely to happen by accident.

@johan johan force-pushed the stringify branch 4 times, most recently from 57686b1 to 230a814 Compare November 29, 2014 06:53
@jkrems
Copy link
Contributor

jkrems commented Dec 1, 2014

The problem I have with bundling a CLI tool is that this library is so far most often used as a dependency/basis for others. And adding a random CLI tool 3 times nested somewhere in the dependency tree just adds bloat. For example atom/season uses cson-safe for parsing and has a very nice CLI tool already.

As for the stringify: So far our interface is (or at least is supposed to be) compatible with the well-documented JSON.* functions. Where it isn't, it's a todo item or a bug. I'd rather not add special casing like "unless you used something that isn't a space or a tab as the third argument to stringify":

JSON.stringify({ a: 'b' }, null, 'xx')
// '{\nxx"a": "b"\n}'

@johan johan force-pushed the stringify branch 2 times, most recently from 692c45a to daa53f6 Compare December 2, 2014 23:11
@johan
Copy link
Contributor Author

johan commented Dec 2, 2014

Heh, ecma-262 literally does specify the first ten any-chars in the space string should be used, ws or not.

Fixed! Also, nuked cli tools and squashed history. (People really wanting either feature can grab'em off of this archival branch.)

n = 1 unless n in [1..10] # do not bail on NaN and similar
Array(n + 1).join ' '

else ' '
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why you are defaulting to two spaces, but I'd like to opt for consistency here - CSON.stringify obj, null, 2 might be a bit more verbose but it's how JSON.stringify works and less surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it doesn't feel like CSON without any of CSON's non-JSON properties, you're probably right. Addressing.

@johan
Copy link
Contributor Author

johan commented Dec 3, 2014

Feedback addressed. It somehow didn't even occur to me that JSON.stringify's default indention would even apply to CSON, but it probably usually ends up the shortest form, which isn't a bad codec default.

@jkrems
Copy link
Contributor

jkrems commented Dec 3, 2014

LGTM - thanks a lot!

jkrems added a commit that referenced this pull request Dec 3, 2014
@jkrems jkrems merged commit 95df466 into groupon:master Dec 3, 2014
@jkrems
Copy link
Contributor

jkrems commented Dec 3, 2014

v0.2.0

@johan johan deleted the stringify branch December 3, 2014 00:37
johan added a commit to johan/season that referenced this pull request Dec 3, 2014
@johan johan mentioned this pull request Dec 3, 2014
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