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

Control Indent Mapping Context #241

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

spgarbet
Copy link

Problem: Downstream consumer of libyaml has some divergent formatting changes. Would prefer to use libyaml without modification. Possibly towards using system installed library: vubiostat/r-yaml#102

It was felt important enough to downstream users to change this bit of formatting on output, so this is important to some users.

Goal: Merge this change in with necessary tests, and understand why the formatting directive was necessary.

Needs: (1) Blessing from dev team. (2) Test cases.

This pull requests has the divergence between the two with one line commented out due to breaking two tests. If the line is commented in the following test snippet occurs with 2 broken tests:

...
ok 89 F6MC: More indented lines at the beginning of folded block scalars
not ok 90 F8F9: Spec Example 8.5. Chomping Trailing Lines
# --- data/F8F9/out.yaml	2022-02-14 09:59:24.889902598 -0600
# +++ /tmp/test.out	2022-02-14 09:59:26.145888379 -0600
# @@ -5,4 +5,3 @@
#  keep: |+
#    # text
#  
# -...
ok 91 FP8R: Zero indented block scalar
ok 92 FQ7F: Spec Example 2.1. Sequence of Scalars
ok 93 FTA2: Single block sequence with anchor and explicit document start
ok 94 FUP4: Flow Sequence in Flow Sequence
ok 95 G4RS: Spec Example 2.17. Quoted Scalars
ok 96 G5U8: Plain dashes in flow sequence
ok 97 G992: Spec Example 8.9. Folded Scalar
ok 98 GH63: Mixed Block Mapping (explicit to implicit)
ok 99 H2RW: Blank lines
ok 100 H3Z8: Literal unicode
ok 101 HMK4: Spec Example 2.16. Indentation determines scope
ok 102 HMQ5: Spec Example 6.23. Node Properties
ok 103 HS5T: Spec Example 7.12. Plain Lines
ok 104 HWV9: Document-end marker
ok 105 J5UC: Multiple Pair Block Mapping
ok 106 J7PZ: Spec Example 2.26. Ordered Mappings
ok 107 J7VC: Empty Lines Between Mapping Elements
ok 108 J9HZ: Spec Example 2.9. Single Document with Two Comments
ok 109 JHB9: Spec Example 2.7. Two Documents in a Stream
ok 110 JQ4R: Spec Example 8.14. Block Sequence
ok 111 JS2J: Spec Example 6.29. Node Anchors
ok 112 K3WX: Colon and adjacent value after comment on next line
ok 113 K4SU: Multiple Entry Block Sequence
ok 114 K527: Spec Example 6.6. Line Folding
not ok 115 K858: Spec Example 8.6. Empty Scalar Chomping
# --- data/K858/out.yaml	2022-02-14 09:59:24.894902541 -0600
# +++ /tmp/test.out	2022-02-14 09:59:26.436885084 -0600
# @@ -2,4 +2,3 @@
#  clip: ""
#  keep: |2+
#  
# -...
ok 116 KMK3: Block Submapping
ok 117 L94M: Tags in Explicit Map
...

@spgarbet
Copy link
Author

Possibly related to #224

src/emitter.c Outdated
@@ -1825,6 +1827,7 @@ yaml_emitter_write_indicator(yaml_emitter_t *emitter,

emitter->whitespace = is_whitespace;
emitter->indention = (emitter->indention && is_indention);
/* emitter->open_ended = 0; */
Copy link
Member

Choose a reason for hiding this comment

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

The test suite is passing although this was commented out. That seems suspicious.

Copy link
Author

Choose a reason for hiding this comment

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

That was a change that exists upstream that I'm trying to harmonize. If I leave that in, it breaks tests. I suspect it may be a reverted change from earlier and can be safely removed.

Copy link
Author

Choose a reason for hiding this comment

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

I removed this.

src/emitter.c Outdated
Comment on lines 1983 to 1986
if (emitter->root_context)
{
emitter->open_ended = 1;
}
Copy link
Member

Choose a reason for hiding this comment

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

See above comment. This is basically a revert from a previous commit, but the test suite doesn't catch that.

Copy link
Author

Choose a reason for hiding this comment

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

I pushed this change up.

@abitrolly
Copy link

abitrolly commented Oct 20, 2022

@perlpunk Hi. Any chance to get this reviewed? Looks useful so that libyaml output can pass default yamllint checks https://yamllint.readthedocs.io/en/stable/configuration.html#default-configuration

Ruby .to_yaml converter is also affected ruby/psych#591

@perlpunk
Copy link
Member

To me the description of the PR isn't really helpful. It has a lot of text and then some testsuite output diff, but it doesn't show any example of what it is doing.
How does the new output look like for various test cases, and various indentations?
(Note that I'm not a maintainer, just a contributor. I don't make decisions about libyaml behaviour.)
I commented earlier mainly because the original PR did some apparently unrelated changes, and I was wondering why the testsuite didn't complain about this.
So several questions for @ingydotnet here.

@abitrolly
Copy link

@perlpunk yea, the diff looks like there an additional flag that disables indentation of sequences. Not sure if it helps ruby/psych#591

@spgarbet
Copy link
Author

spgarbet commented Jun 1, 2023

I have the code from this library copied into a project, r-yaml. The only change at present is this indentation change. If I take it out, my issue bucket lights up with complaints. I NEED this behavior. It'd be nice to enable it somehow either via compile or via flag. Then the library in the r-yaml project would not be divergent.

@abitrolly
Copy link

@spgarbet if you can provide an example of output without the patch and with it, it would be most helpful to understand what the PR does.

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.

None yet

3 participants