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

compact seq indent option #753

Closed
wants to merge 4 commits into from

Conversation

natasha41575
Copy link

@natasha41575 natasha41575 commented Jun 22, 2021

fixes #720, needed for kustomize to avoid breaking changes when upgrading our yaml v3 dependency.

This PR also includes a needed fix for #755 in its last commit, but I can remove it if preferred. The commit in #756 is the same as the last commit in this PR.

Happy to update the PR to expose the flag in a different way if that's desired.

Same questions as #750:

  1. @niemeyer - does this align with your approach?
  2. Should we add more tests? Any cases that are crucial to cover?

go.sum Outdated
@@ -0,0 +1,4 @@
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
Copy link

Choose a reason for hiding this comment

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

I noticed that @niemeyer didn't have a go.sum. It might be that the build or whatever else produces it. I would not introduce this file, it seems like an unrelated change.

Copy link
Author

Choose a reason for hiding this comment

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

done

yaml.go Outdated
@@ -278,6 +278,11 @@ func (e *Encoder) SetIndent(spaces int) {
e.encoder.indent = spaces
}

// DisableSeqIndent disables the indentation for sequence items.
func (e *Encoder) DisableSeqIndent() {
e.encoder.emitter.disable_sequence_indent = true
Copy link

Choose a reason for hiding this comment

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

is there a way this can be called prior to the encoder and emitter being instantiated?

Copy link
Author

Choose a reason for hiding this comment

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

good question, though I'm wondering why we would want to do that?

Copy link

Choose a reason for hiding this comment

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

I don't think we wold want to do that, but in general as a package designer one should anticipate that people will do things that don't make sense. Is there a way to run into trouble with this interface? Can we avoid it? My PR had a flag on the current struct and then set one on the internal struct at init().

I guess it's not a huge deal and we can see if people break themselves first.

@natasha41575 natasha41575 changed the title disable seq indent option compact seq indent option Jun 23, 2021
@natasha41575
Copy link
Author

Still interested in seeing this merged, but closing due to lack of activity. Please let me know if you have any interest in continuing this discussion.

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.

2 participants