feat/fix: support literal and single-quoted style for YAML::Binary emitting#1447
Conversation
|
I also implemented the line wrapping as mentioned in #1287, should I commit it here or open another PR? |
SGSSGene
left a comment
SGSSGene
left a comment
There was a problem hiding this comment.
Thank you for this PR!
There are two concerns:
- The code that does the switching is hard to read and check for correctness. Some (non-trivial) comments could be helpful.
- Why does the indentation for pointing float type change?
| m_pState->CurIndent() + m_pState->GetIndent()); | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add some comment for this block, what actually is happening?
Why do you format create a format string for "a"?
And why is there a case for case StringFormat::Plain that is commented out?
There was a problem hiding this comment.
About "a"：
Just like Emitter::Write for const char*, we should use ComputeStringFormat to generate StringFormat. And I also thought it necessary to offer functions like WriteSingleQuotedBinary and WriteLiteralBinary in Utils and call them, rather than directly generating base64 and calling Utils functions for strings. However, ComputeStringFormat indeed requires a string, but the base64 string is not generated in Emitter::Write! If we encode twice, it may cost double the time with long binary.
In fact ComputeStringFormat returns the same value for any non-empty base64 string and a, so I just passed a for all non-empty Binary.
Maybe adding a ComputeBinaryFormat will look better?
About Plain：
Previously, without any options, the strFormat remains Plain by default but emits double-quoted style for Binary. Some long-existing tests are also based on this. Changing the default behavior may also influence some downstream projects. So I left it unchanged (still double-quoted).
There was a problem hiding this comment.
Best appreciation to you!
| "foo: .inf\n" | ||
| "bar: .inf"); |
There was a problem hiding this comment.
Could you explain why the indentation of these change?
There was a problem hiding this comment.
This may be caused by clang-format. Previously this case used tab for indentation and the formatter changed it to spaces.
There was a problem hiding this comment.
This may be caused by clang-format. Previously this case used tab for indentation and the formatter changed it to spaces.
Ah yes! than just leave the fix in. I just noticed other PRs have the same change.
|
Now it may look better. |
This I find much easier to follow and read. Thank you! |
2 participants
Fixes #1445 and a part of issue #1287.
Support literal and single-quoted style for YAML::Binary emitting. Attempts to support plain form may change the default behavior, so are not applied.
Related tests are added to
EmitterTest.BinaryStylesin test/integration/emitter_test.cpp.