stringify should accept option to use single rather than double quotes

[samestep]

Similar to this issue in cson, but as stringify is now implemented in this library, this is where the issue should be resolved.

I propose that a singleQuote parameter be added to the signature of stringify and propagated to the signatures of internal functions as needed, and that in these two lines, JSON.stringify be replaced with

singleQuote ? singleQuoteStringify : JSON.stringify

where singleQuoteStringify converts a JavaScript value to a single-quoted ES string literal.

I believe this change is justified because it allows this library to be used to resolve issues such as those that arise in maintaining an Atom config.cson file. I realize that this would remove the property that the interface is identical to JSON.stringify, but since this change adds an parameter to the end of the interface, and the behavior remains the same when that argument is omitted, the phrase "identical to" there could be changed to "compatible with" with no adverse consequences.

Would the maintainers of this library be interested in such a change? If so, I will implement the singleQuoteStringify function, adjust the function parameters and those two lines, add tests, and submit a pull request.

[jkrems]

Hi! Thanks for bringing this up! The reason it's using double-quotes really comes down to "it was easier to implement".

Does this actually solve the problem though? The post describes auto-formatting. So even if we would add the option, it would just reverse the problem..?

I would be fine with either solution (changing the default to single quotes or adding an option). I might be leaning towards changing the default and bumping the major because I'm not a huge fan of doubling the surface area for our test suite(s).

[samestep]

Right, I looked around and I wasn't able to find an already available solution for stringifying with single quotes, so it makes sense to use double quotes because JSON.stringify already gives those and they're compatible with CSON.

I'm not entirely sure what you mean about reversing the problem... Ideally I'd like to be able to set an option in my Atom config.cson that says "use this quote strategy when writing config.cson", but if I can't get that implemented, then I'd like to write a little shell script that reads in my config.cson, parses it and stringifies it using this library, and writes it back out so that I don't have to manually change the quotes every time I commit a change to my Atom config. In either of those cases, this library would need to support single quotes in stringify.

Actually, thinking about it more, I think that a single boolean flag for switching from single to double quotes isn't good enough. For instance, in my Atom keymap, I use double quotes for one string because the contents of that string are just a single quote, so I avoid the need to do any escaping. Therefore, I would instead propose that the fourth parameter of stringify be a function parameter called quotes, and that JSON.stringify be replaced in those two lines with this function:

function(string) {
  return (
    quotes && quotes(string) === 'single'
    ? singleQuoteStringify
    : JSON.stringify
  )(string);
}

Then the user could pass in a function like this for quotes:

function(string) {
  function contains(string1, string2) {
    return string1.indexOf(string2) >= 0;
  }
  return contains(string, "'") && !contains(string, '"') ? 'double' : 'single';
}

[samestep]

With which ES versions does this library need to remain compatible, by the way?

[jkrems]

Right now it's officially supporting back to node 0.10 (see .travis.yml). I'd be fine with dropping 0.10 support but that would mean bumping the major for this change.

And if we're bumping the major anyhow, do you want to just add the logic you have up there directly in the library? Seems fairly reasonable ("prefer single unless the string contains a single quote but no double quotes"). We already do that kind of switching for new-lines. I don't see a reason not to do it for quote style as well.

If you feel strongly that it should be configurable, then the configuration should also handle multi-line options ('single-multiline' vs. 'double-multiline') for consistency.

[samestep]

I agree, I think that logic is reasonable for a default setting. It's up to you whether to do that or add a quotes configuration function parameter to stringify. That's a good point about the multiline strings; also, do you know if there's a spec for CoffeeScript multiline string literals, or for that matter, for arbitrary CoffeeScript string literals? I was planning on referring to the ES spec for writing singleQuoteStringify, but I figure that while we're making this change anyway, I'd like to take a look at the broader context of different valid stringifications.

[jkrems]

IIRC the spec correctly, the only difference between single- and double-quoted strings is escaping the reverse character. So I would expect it to be as easy as starting with the result of JSON.stringify and just switching out the two patterns. But I might be underestimating how complex that'd be.

Do you know if there's a spec for CoffeeScript multiline string literals, or for that matter, for arbitrary CoffeeScript string literals.

As far as I'm aware the only official "spec" for CoffeeScript is the only/reference implementation. Not sure if that changed in the meantime though.

[samestep]

Ah, thanks! I was going to write it without using JSON.stringify, but this makes more sense. I wrote an initial singleQuoteStringify implementation in a new branch. At this point I can either make the above behavior the default or introduce a quotes function parameter; did we end up deciding which of those options to take? Also, how can I run the tests? There doesn't seem to be anything about that in the README.

[jkrems]

At this point I can either make the above behavior the default or introduce a quotes function parameter; did we end up deciding which of those options to take?

I'd say just update the default behavior.

Also, how can I run the tests? There doesn't seem to be anything about that in the README.

We might want to add it to the README or CONTRIBUTING but the quick answer is npm test should just work.

[samestep]

Done. Are there any further tests I should add?

[jkrems]

Closed via #63