carvel-dev / ytt

YAML templating tool that works on YAML structure instead of text
https://carvel.dev/ytt
Apache License 2.0
1.62k stars 136 forks source link

Trailing comment affecting `yamlfmt.Printer` #911

Open prnvbn opened 2 months ago

prnvbn commented 2 months ago

What steps did you take: I have the following Go program and YAML file. The Go file integrates with the ytt API and reads the ytt yaml file and just prints it put to standard output. I would expect the file to be printed out with no semantic differences. However, that is not the case!

import ( "os"

"carvel.dev/ytt/pkg/yamlfmt"
"carvel.dev/ytt/pkg/yamlmeta"

)

func main() { bs, := os.ReadFile("example.yaml") docset, := yamlmeta.NewDocumentSetFromBytes(bs, yamlmeta.DocSetOpts{}) printer := yamlfmt.NewPrinter(os.Stdout) printer.Print(docset) }


- YAML file: I have provided a very minimal YAML file
```yaml
hello: world
# test

What happened: I see the following in my standard out.

hello: world

# test
---

What did you expect: I expected the following in my stdout

hello: world

# test

Anything else you would like to add: I am pretty sure the trailing comment is causing this issue.

Environment:

module issue

go 1.22

toolchain go1.22.2

require carvel.dev/ytt v0.49.0

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible" 👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help to work on this issue.

praveenrewar commented 1 month ago

@prnvbn Apologies for the late reply. Could you please share some more context around what you are trying to achieve here? By default ytt would remove this comment (ytt -f example.yaml) as this is not a template file (doesn't have any ytt comments). You can achieve the same result by passing WithoutComments as true in DocSetOpts if the comment is not required.

renuy commented 1 month ago

@prnvbn Thanks for raising this. We are adding this to our backlog, however if you want to fix this please raise a PR and we will happy to review it on priority.