Skip to content

yqutil: fix to preserve line breaks #2625

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

Merged

Conversation

norio-nomura
Copy link
Contributor

@norio-nomura norio-nomura commented Sep 17, 2024

yamlfmt works by replacing line breaks with place holder strings to avoid their loss when using gopkg.in/yaml.v3. By replacing line breaks with place holder strings before yqlib uses gopkg.in/yaml.v3, we can ultimately prevent the loss of line breaks through yamlfmt.

#2593 (comment)

`yamlfmt` works by replacing line breaks with place holder strings to avoid their loss when using `gopkg.in/yaml.v3`. By replacing line breaks with place holder strings before `yqlib` uses `gopkg.in/yaml.v3`, we can ultimately prevent the loss of line breaks through `yamlfmt`.

Signed-off-by: Norio Nomura <[email protected]>
…nothing)'`

`find examples -name '*.yaml' -exec limactl edit --set 'del(.nothing)' {} \;`

Signed-off-by: Norio Nomura <[email protected]>
@norio-nomura norio-nomura marked this pull request as ready for review September 17, 2024 13:09
Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Needs more comments and acknowledgement of the copied upstream code.

contentModified, err := replaceLineBreaksWithMagicString(content)
if err != nil {
return nil, err
}
tmpYAMLFile, err := os.CreateTemp("", "lima-yq-*.yaml")
if err != nil {
return nil, err
}
tmpYAMLPath := tmpYAMLFile.Name()
defer os.RemoveAll(tmpYAMLPath)
Copy link
Member

Choose a reason for hiding this comment

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

I know this is not part of this PR, but why is this calling os.RemoveAll() instead of os.Remove() given that tmpYAMLPath is a file and not a directory.

@afbjorklund Do you remember why you choose RemoveAll?

@norio-nomura norio-nomura force-pushed the yqutil-fix-preserve-line-breaks branch from 7884213 to f2d6a55 Compare September 18, 2024 05:01
@norio-nomura
Copy link
Contributor Author

I rewrote it to avoid relying on yamlfmt's internal code.

Copy link
Member

@jandubois jandubois left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@jandubois jandubois merged commit 95d1863 into lima-vm:master Sep 18, 2024
27 checks passed
@jandubois jandubois added this to the v1.0 milestone Sep 18, 2024
@norio-nomura norio-nomura deleted the yqutil-fix-preserve-line-breaks branch September 18, 2024 23:44
@norio-nomura
Copy link
Contributor Author

Thanks! 🙏🏻

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