jbeder / yaml-cpp

A YAML parser and emitter in C++
MIT License
5.15k stars 1.85k forks source link

a YAML::NewLine at the end of a sequence brake the next node #902

Closed Raffaello closed 4 years ago

Raffaello commented 4 years ago

eg:

having a sort of this code snippet:

out << YAML::Key << "k" << YAML::Value
                    << YAML::Flow << YAML::BeginSeq;
                for (int i = 0; i < 2; i++) {
                    out << YAML::BeginMap
                        << YAML::Key << "i" << YAML::Value << i
                        << YAML::EndMap 
                        << YAML::Newline
                        ;
                }
                out << YAML::EndSeq;

if that node is running through another outer loop of Begin/End Map is generating a sort of this result:

nodeA:   
    k: [{i: 0},
 {i:1},
  ]NodeB:
    k: [{i: 0},
 {i:1},
  ]

instead without the YAML::NewLine is:

nodeA:  
    k: [{i: 0}, {i:1}]
  NodeB:
    k: [{i: 0}, {i:1}]

it seems that the last new line before ending the sequence should be "ignored" and is not, breaking the next node alignment and resulting in an invalid YAML file.

of course there is a workaround in the code, and just avoid the last new line to be emitted.

but is very not elegant and require to do n-1 in the loop and repeat the inside loop block without the newline, a better solution would be nice if the yaml-cpp library handle that edge case, basically removing the newline (or ignoring it) when the EndSeq is emitted.


Raffaello commented 4 years ago

it seems that the new line change a global state and not just the child state of that specific sequence: https://github.com/jbeder/yaml-cpp/blob/72fe73a1043bef4a4f9e7032132f2aa50865d97e/src/emitter.cpp#L253

and then: https://github.com/jbeder/yaml-cpp/blob/41001d1bf9c579598d10ce7b2a3552bebe8a7155/src/emitterstate.cpp#L56

and then when i start the "nodeB" as a map the method hasBegun return true, when instead should return false in my perspective at least:

https://github.com/jbeder/yaml-cpp/blob/0d5c57150cebb430fcc216d8066dbb28aeaafe8e/src/emitterstate.h#L69-L71


could it be a wrong "restore" operation among the stack of children? i mean in the specific case above it pop twice (endseq and an endmap) before starting the new nodeB, and the status is still "has_begin" so when an end of something shouldn't reset the flag(s)/status hasBegun to false? (in this case setting all those variable to false?) just by adding for eg a setEnded() that is just set all of those variable to false?

Raffaello commented 4 years ago

~it seems alsto the method IndentTo(indent); could potentially have an assert: assert.col()<= indent not sure if it is that intention or can just have an higher col position than the indent.~

jbeder commented 4 years ago

Yes, this is definitely a bug, but it's not obvious how to fix it. Open to suggestions.

On Mon, Jun 22, 2020 at 9:23 AM Raffaello Bertini notifications@github.com wrote:

it seems alsto the method IndentTo(indent); could potentially have an assert: assert .col() <= indent not sure if it is that intention or can just have an higher col position than the indent.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jbeder/yaml-cpp/issues/902#issuecomment-647551743, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAICUBU7M6GVLAX6UAMJKV3RX5SPLANCNFSM4OESR3VA .

jbeder commented 4 years ago

Ah, I see you submitted a PR. Reviewed.

jbeder commented 4 years ago

Fixed by #903