Skip to content

#2298 change yaml parser to a maintained one #2389

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

Conversation

Mivr
Copy link

@Mivr Mivr commented Jun 4, 2025

Hello,

I did some work on changing the parser to goccy yaml instead of the unmaintained one, tests are passing and it seems to be working.

Do have in mind that goccy yaml does not preserve the order of fields, so this we can either fix in yq or consider a breaking change.

The current implementation is behind a flag so to not break users.

Note: I am a somewhat skilled go developer, but I haven't touched in years. Most of the code in this PR is generated with AI and then manually checked by me.

Fixes: #2298

@ccoVeille
Copy link

I would say it's urgent to wait for the fork that the YAML organization is now officially supporting

https://github.com/yaml/go-yaml

@Mivr
Copy link
Author

Mivr commented Jun 4, 2025

Oh, in that case, maybe a PR to remove goccy intial integration and use the new fork is relevant? (of course once the package is published)

And maybe I will make a PR to them for the case of "?" handling in go-yaml.

@ccoVeille
Copy link

Here,I would let a maintainer reply

But I wanted to raise to anyone attention that yaml/go-yaml exists

@mikefarah
Copy link
Owner

Good to see that go-yaml is getting support again - will be interesting to see how much activity it gets and how many issues they fix over the next couple of months.

I'm not too sure on this PR - I'm currently working on the goccy decoder/encoder (albeit slowly because life 😅 ) would be good to get it done; but there's a lot of code here I'll need to work through.

Not sure about your comment of not preserving order - that would definitely be a deal breaker; but as far as I can tell it does preserve order.

Nonetheless, I'll be going on an extended holiday soon; will look at this (and progression of go-yaml) when I get back. Till then I won't be making fundamental changes or decisions on yq :)

@mikefarah
Copy link
Owner

Btw - to really know if the decoding/encoding work properly, you'll need to change the default encoder and decoder in operator.test (e.g. https://github.com/mikefarah/yq/blob/master/pkg/yqlib/operators_test.go#L56) to see if goccy can handle all the various scenarios

@Mivr
Copy link
Author

Mivr commented Jun 6, 2025

I see that go-yaml might have a version soon, so maybe keeping Goccy behind a flag is a good maybe short term solution as this will unblock some other issues like this one: aspect-build/rules_js#2100

@Mivr
Copy link
Author

Mivr commented Jun 6, 2025

@mikefarah I think I fixed most of it, a lot of tests had to be ignored, do say what you prefer to do with the different parsing in goccy, for now its just an ignore but I can expect a better approach is needed

@mikefarah
Copy link
Owner

Having tests ignored will produce more problems than it will solve

@mikefarah
Copy link
Owner

Oh you've just updated the test expactation to match what the AI code generated :( Sorry but this is never going to work like that.

@mikefarah mikefarah closed this Jun 7, 2025
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.

Replace go-yaml/yaml with goccy/go-yaml
3 participants