Add single and double quote styles #242
Conversation
2bbaa55 to
e3213c8
Compare
Codecov Report
@@ Coverage Diff @@
## master #242 +/- ##
==========================================
- Coverage 78.39% 77.59% -0.81%
==========================================
Files 12 13 +1
Lines 3421 3494 +73
==========================================
+ Hits 2682 2711 +29
- Misses 505 546 +41
- Partials 234 237 +3 |
e3213c8 to
7bee600
Compare
| case QuoteStyleSingle: | ||
| v = quoteWith(v, '\'') | ||
| default: | ||
| v = strconv.Quote(v) |
There was a problem hiding this comment.
This is pretty much the same as calling quoteWith now. Should we just call quoteWith?
| package yaml | ||
|
|
||
| import ( | ||
| "unicode" |
There was a problem hiding this comment.
The original code didn't import unicode to avoid bloating the binaries that import it. Is that a concern for go-yaml too? We can copy their custom isPrint function if necessary.
| v = strconv.Quote(v) | ||
| switch e.quoteStyle { | ||
| case QuoteStyleSingle: | ||
| v = quoteWith(v, '\'') |
There was a problem hiding this comment.
This is difficult. In the original Issue, the submitter worried that originally defined string in a single quote changes to double quoted string at marshal. In this case, I think it is enough to always be able to select single quote or double quote for "quoted string" ( e.g. contains special character like { or } ) .
For example, when outputting a character containing a new line code ( \n or \r ), I think that using Literal ( | ) is more readable than a quoted character string.
There was a problem hiding this comment.
Ok, yeah:
- I'll mark this PR as not actually closing the linked issue. It solves a different issue: I wanted to be able to turn all double quoted strings in the output into single quoted strings, so definitely not the same as the issue I referenced.
- There's a much bigger discussion to be had about how quoting should behave. It seems like right now, the quoting is not very smart and it needs to get much fancier. I'd love to chat about this synchronously rather than over github comments because the discussion would take forever this way. Let me know your preferred method of communication. You can DM me on twitter at @vikstrous
There was a problem hiding this comment.
OK ! I would like to discuss on gophers.slack.com . I created #goccy-go-yaml channel .
Could you join it ?
| ) | ||
|
|
||
| // QuoteStyle specifies how strings should be quoted | ||
| func QuoteStyleOpt(qs QuoteStyle) EncodeOption { |
There was a problem hiding this comment.
What do you think WithQuote(QuoteStyle) EncodeOption ( or Quote(QuoteStyle) EncodeOption ) as name candidate ?
|
Since we can't talk synchronously, here are my thoughts and questions:
|
|
Ping? What should I do? I want to get this merged somehow. |
I'm looking for feedback on the naming. It's not great that I had to add Opt to the end of the name of the option because the other options don't have that. It would be great if we can avoid that somehow.
I'm also wondering if there will be other quote related options in the future that we should plan for. It wouldn't be great to have combinatorial explosion of different quote style enums. Maybe I should make this option more specific than "quote style"? Maybe we can just have a bool with something like "SingleQuoteStrings"?