cmd/export: emit comments by default for yaml

[felixge]

Closes #1180

[rogpeppe]

This looks fine in general to me (and welcome!), but one thought: cue export --out cue already preserves comments, so maybe this should just be the default behaviour rather than adding another command-line flag? I suspect that most people might prefer the comments to remain. Or maybe the sense of the flag should go the other way, to deliberately suppress comments (so that would mean something for cue export --out cue too). WDYT @mpvl ?

[felixge]

@rogpeppe Hi 👋. Your comment makes sense to me. I'd also prefer this to be the default, with a flag like --no-comments for opting out. But happy to take this in any direction @mpvl deems right.

[mpvl]

@rogpeppe @felixge: I'm a bit ambivalent about what the default should be, but at the very least it should be consistent with the behavior for other formats. Since the default for cue is to output comments, I agree the same should probably be true for yaml as well.

Note that for cmd eval we already have the flags show-(hidden|optional|attributes). The plan is to replace cue eval with a more principled version as the "default" command (cue). Assuming will will need flags for this there too, the comments flag should be consistent with this. I'm fine with a different CLI, though, for instance one where there is one flag, like --show, that takes an argument that describes toggles for each. E.g. --show=nocomments or --drop=comments.

For now, though, for consistency, it may be easiest to use --show-comments=false to disable the comments.

An alternative thought would be to overload the "simplify" flag (-s) or to have a --compact flag or so, as a catchall for omitting compact output, like omitting comments. This flag would be some compact form that is specific to a format. For yaml this could mean no comments. For json this could mean no indentation. Just a thought.

The advantage of such a flag is that it may be useful in general, while allowing us to punt for now on proper naming for the flag specific to comments.

[rogpeppe]

I'm not sure I'd support overloading -s in that way, as it's quite possible one might want to produce simplified expressions but still with comments attached. However --compact sounds like a nice idea - I suspect that the main reason people might wish to elide comments is to obtain smaller output.

[felixge]

note: unrelated fix for a problem I noticed

[felixge]

Based on the discussion above, I just updated this PR to emit comments by default. I have not added --compact flag yet – but it SGTM. Please confirm @mpvl. cuelang.org/go/doc/tutorial/kubernetes seems to fail now, but this should be an easy fix once we agree on the command line flags / defaults.

One thing I'd like to note: I see two use cases for commenting .cue files:

1. Explain certain nuanced usages of the cue language itself to achieve a certain output. Preserving those comments in the final YAML/JSON is likely undesirable in all circumstances.
2. Comments directly describing the YAML/JSON output and how it will be interpreted by the consumer. Those comments are probably desirable by default and would only need to be omitted in scenarios where no human might need to inspect the resulting JSON/YAML.

In the long run it would be nice to make cue aware of the different types of comments described above, but for now I think emitting all kinds of comments by default for YAML would be an improvement.

[marcus-crane]

Just stopping by to say I was just discussing this with a coworker and it'd a cool feature!

2. Comments directly describing the YAML/JSON output and how it will be interpreted by the consumer.

If it helps, our probable use case (just proposed Cue internally yesterday) is that we use it to validate some internal JSON files (probably will change them to YAML) that we then currently generate Terraform code off of. The JSON files contain various flags to determine say; you'd like to generate a service with a public load balancer vs a private one. The file abstracts away the specifics which is also handy given Cue lets us toggle different things on and off based on fields. For example, if type === daemon, we don't need a load balancer.

In the case of comments, it would be nice to fully document the resulting YAML file so that users know what all of the possible inputs/options are when they go to update the generated templates. The general proposed flow is that we (ab?)use Cue by exporting a default config file via cue export that just has all of the configured defaults. We then run any deviations through cue vet to validate that they're, well, valid before running our existing generator code.

Historically, we've just had a JSON file with no consistency across older generated files and newer ones that have newly added features. Backporting Cue comments would be cool as well by rerunning Cue over them 🙂

Anyway, personally I don't mind if this is behind a flag or on by default but yeah, cool to see this PR!

[rogpeppe]

I'm sure this isn't the fault of this PR, but there's definitely a bit more work to do here.
Some comments can be lost. For example:

[{
	// A comment

	name: "Foo"
}]

when the above exported as YAML using the preserve-comments branch, the comment is lost.
If the blank line is removed before the name field, the comment remains, but the
position of the comment is arguably non-idiomatic (I'd expect it to go on a line of its own):

- # A comment
  name: Foo

[rogpeppe]

Another place a comment is lost on export:

{
	name: "Foo"	// A comment
}

I also noticed that end-of-line comments are also lost on import, so the comment in the following YAML is lost:

name: Foo 	# A comment

[rogpeppe]

One other thing perhaps worth mentioning (might not be fixable): say we import some YAML like this:

# Header comment
- name: foo
  value: x
- name: bar
  value: y

It imports to CUE like this:

[{
	// Header comment
	name:  "foo"
	value: "x"
}, {
	name:  "bar"
	value: "y"
}]

and then when re-exported as YAML, it looks like this:

- # Header comment
  name: foo
  value: x
- name: bar
  value: "y"

That is, what was originally intended as a comment on the entire list of items has turned into
a comment on just the first item in the list.

[felixge]

@rogpeppe great points. I'd argue whether a comment above a list belongs to the first item or the list is ambiguous. Not sure what the right solution is.

Anyway, I think these edge cases are somewhat orthogonal to the preservation of comments in general (this PR).

But if this can't be merged until the whole approach is sound, I'm okay with it. I just don't have time right now to contribute to the nuanced debate and implementation this will require :(.

[rogpeppe]

After chatting this through with @mpvl, I think that this PR is probably fine as is. There are some hard problems to solve to get comments on output really right (the general problem probably doesn't even have a "right" answer in fact), and this will be a good way forward.

[felixge]

@rogpeppe awesome. I just tried to fix the failing tests for this PR, but I could use some help with updating the golden files, e.g. the stuff in ./cmd/cue/cmd/testdata/script/cmd_github.txt.

Here is what I tried:

$ cd internal/ci
$ go generate
$ cue cmd updateTxtarTests

But unfortunately this only modified the github workflow files, no the the golden fixtures:

	modified:   ../../.github/workflows/release.yml
	modified:   ../../.github/workflows/repository_dispatch.yml
	modified:   ../../.github/workflows/test.yml
	modified:   ../../.github/workflows/tip_triggers.yml

Please advise : )

[shoukoo]

Also interested in this feature, just wondering if anything I can do to help with merging this feature?

[myitcv]

Paging @rogpeppe for an update on this.

[mvdan]

I just tried to fix the failing tests for this PR, but I could use some help with updating the golden files

In general, you can update all the generated and "golden" files (even inside txtar archives) via:

go generate ./...
CUE_UPDATE=1 go test ./...

Hopefully that helps? There's also a minor conflict now, if you could rebase that would be helpful.

[mvdan]

I didn't get a response above so I'll take care of importing this to Gerrit while fixing the merge conflicts and the test files :) I'll keep Felix as the author of course.

[mvdan]

Importing this into Gerrit uncovered a bit of a design issue. See #1180 (comment); I will post updates on that thread from now on. Please direct any design discussion there as well.