Draft: #2298/change yaml parser to a maintained one #2392

Mivr · 2025-06-07T08:50:11Z

No description provided.

ccoVeille

Minor remark, more from an end user than a real code review

ccoVeille · 2025-06-07T09:45:26Z

cmd/root.go

+			switch yamlParser {
+			case "goccy", "":
+				yqlib.ConfiguredYamlPreferences.UseGoccyParser = true
+			case "v3":


I would call it "legacy-v3", or simply "legacy"

It should be obvious this option is no longer maintained

Good note, thanks will update

… place, add datetime preprocessor for date time difference

…l.v3 preserves them while goccy normalises them)

ccoVeille · 2025-06-09T12:05:16Z

pkg/yqlib/datetime_preprocessor.go

+	// Try to parse date-only format
+	if _, err := time.Parse("2006-01-02", value); err == nil {
+		return true
+	}
+
+	// Try RFC3339 without timezone
+	if _, err := time.Parse("2006-01-02T15:04:05", value); err == nil {
+		return true
+	}


There are constants in time package for this.

You can use usestdlibvars linter to identify them easily

Surprisingly usestdlibvars did not show any errors, maybe my setup has a problem...

Anyway fixed (next commit)

ccoVeille · 2025-06-09T12:06:41Z

pkg/yqlib/datetime_preprocessor.go

+	// Skip empty lines, comments, directives, document separators
+	if trimmed == "" || strings.HasPrefix(trimmed, "#") ||
+		strings.HasPrefix(trimmed, "%") || strings.HasPrefix(trimmed, "---") ||
+		strings.HasPrefix(trimmed, "...") {
+		return true
+	}
+


I would suggest a switch with one case (or multiple) it would be more readable

ccoVeille · 2025-06-09T12:06:59Z

pkg/yqlib/datetime_preprocessor.go

+	if strings.HasSuffix(trimmed, "|") || strings.HasSuffix(trimmed, ">") ||
+		strings.HasSuffix(trimmed, "|-") || strings.HasSuffix(trimmed, ">-") ||
+		strings.HasSuffix(trimmed, "|+") || strings.HasSuffix(trimmed, ">+") {
+		return true
+	}


ccoVeille · 2025-06-09T12:08:05Z

pkg/yqlib/datetime_preprocessor.go

+		strings.HasPrefix(trimmedValue, "\"") ||
+		strings.HasPrefix(trimmedValue, "'") ||
+		strings.HasPrefix(trimmedValue, "{") ||
+		strings.HasPrefix(trimmedValue, "[") ||
+		strings.HasPrefix(trimmedValue, "&") ||
+		strings.HasPrefix(trimmedValue, "*") {


Using a switch on trimmedValue[0] would be more readable

ccoVeille · 2025-06-09T12:08:22Z

pkg/yqlib/datetime_preprocessor.go

+	// Skip if value is empty, quoted, or already complex
+	if trimmedValue == "" ||
+		strings.HasPrefix(trimmedValue, "\"") ||
+		strings.HasPrefix(trimmedValue, "'") ||
+		strings.HasPrefix(trimmedValue, "{") ||
+		strings.HasPrefix(trimmedValue, "[") ||
+		strings.HasPrefix(trimmedValue, "&") ||
+		strings.HasPrefix(trimmedValue, "*") {
+		return line
+	}


pkg/yqlib/datetime_preprocessor.go

ccoVeille · 2025-06-09T12:10:11Z

pkg/yqlib/decoder_goccy_yaml.go

+	var commentLineRegEx = regexp.MustCompile(`^\s*#`)
+	var yamlDirectiveLineRegEx = regexp.MustCompile(`^\s*%YA`) // e.g., %YAML


Move this out, otherwise the regexp is computed each time the function is called

pkg/yqlib/lib.go

ccoVeille · 2025-06-09T12:27:53Z

pkg/yqlib/datetime_preprocessor.go

+// ISO8601 date/time patterns that should be automatically tagged as timestamps
+var dateTimePatterns = []*regexp.Regexp{
+	// RFC3339 / ISO8601 with timezone: 2006-01-02T15:04:05Z or 2006-01-02T15:04:05+07:00
+	regexp.MustCompile(`^\s*([0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(?:\.[0-9]+)?(?:Z|[+-][0-9]{2}:[0-9]{2}))\s*$`),
+	// Date only: 2006-01-02
+	regexp.MustCompile(`^\s*([0-9]{4}-[0-9]{2}-[0-9]{2})\s*$`),
+	// RFC3339 without timezone: 2006-01-02T15:04:05
+	regexp.MustCompile(`^\s*([0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}(?:\.[0-9]+)?)\s*$`),
+}


I would move this aside the function that use it

Mivr · 2025-06-11T21:16:10Z

@ccoVeille Thank you very much for the review. I will be addressing the comments tomorrow.

And for the parser change, Goccy handles comments differently and this will break workflows for some users. I managed to equalize the behavior around timestamps and dates, but the comments will brake stuff. Also some arrays and anchors are handled differently, which will change the print out but the structure will be kept. So logical flows should not be impacted in contrast to the comments difference where if someone depends on some comment being attached to a specific node, his flow will break with the Goccy parser.

I will make a bit longer explanation of all the differences that I found and how they are addressed in this PR. Of course I am very opened for suggestions and comments. I will be working on this this and next week.

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>

mikefarah · 2025-06-11T23:45:45Z

You're racing ahead here, I'd slow this down a little - the go-yaml parser now has the support of the official Yaml org - I've been chatting with them and they will be soon addressing the issues raised. I'd prefer to keep go-yaml and maintain comments, anchors, array and formatting etc.

Mivr · 2025-06-16T13:57:48Z

@mikefarah

Thesis

You do make a good point: As I get it: Why to work on this if we can just wait for https://github.com/yaml/go-yaml and then just replace a dependency (theoretically).

I can state an argument for having a goccy parser as an alternative behind a flag:

go-yaml has been unmaintained for years, so the actual backlog of issues that need addressing will be probably more than even the one in the now archived repo (~300 open issues). Assuming the YAML foundation continues working on this project with more or less the same speed as the last few months (~10 PRs merged for 2 months) they will have to work for years before they catch up on all the issues goccy has already solved.
- In short my first point is that go-yaml even maintained by a very serious company is still years behind other implementations. So the time for getting a reliable parser is not the time it takes to release the first version of go-yaml, but the time it takes to fix a 3+ year legacy codebase with >300 issues opened.
goccy can easily work behind a flag with a warning for known and possibly unknown incompatibilities, anyway the people that cannot use go-yaml and need goccy don't have much choice. They are either not working with yq because of go-yaml or they use goccy with some differences to standard yq.
goccy discrepancies are not so many nor very intrusive (more details below).

go-yaml vs goccy parsing differences (not mitigated already in this PR)

The differences between go-yaml and goccy displayed visually:

1. Comments association on array elements (BREAKING change if someone depends on comments inside the yaml file)

Can be mitigated with a pre processor that moves the comments so the user gets the same result in the end.

Input:

name:
  # under-name-comment
  - first-array-child

In this example go-yaml associates the comment with the first element of the array, while goccy associates the comment with the array itself

2. Flow maps will be expanded:

For input:

{a: 1, b: 2}

go-yaml produces the same as output.

goccy produces:

a: 1
b: 2

No workflows will be broken as the data structure is the same, formatting is the only change here.

It can be handled in some complicated pre + post processing but fairly I don't see this as a braking change, no one should depend on the formatting of the output yaml.

3. Merge anchor

Given:

foo: &foo
  a: foo_a
  thing: foo_thing
  c: foo_c

foobar:
  c: foobar_c
  <<: *foo
  thing: foobar_thing

go-yaml output for .foobar:

c: foobar_c
!!merge <<: *foo
thing: foobar_thing

Goccy output for .foobar:

<<:
    a: foo_a
    c: foo_c
    thing: foo_thing
c: foobar_c
thing: foobar_thing

For me goccy seems to be the cleaner more informative output, again if you really want to this can be handled in post processor.

4. Duplicate keys

go-yaml allows for duplicate keys
goccy does not

I think this is a feature rather than a bug. Again pre-processing can be used to level the behavior if this is what you believe is best.

5. Goccy is stricter on some invalid YAML cases

I think this is a feature.

Conclusion

I do think goccy should be supported behind a flag for the use cases that have been waiting for years for a fix in go-yaml and the same will have to wait for years more even with the support of the YAML foundation.

Not implementing goccy as a parser or any other parser than go-yaml forces anyone that want to use yq to fix go-yaml and wait for their changes to be merged, which can take months or years even if they have the capacity to fix go-yaml.

More or less a parallel parser can save hundreds to thousands of work hours for the users of yq.

Mivr · 2025-06-16T13:59:05Z

P.S. if you see value in having goccy as a parser and you want some or all of the differences to be handled with pre and/or post processors please do write so and I will add them to this PR.

LarsStegman · 2025-06-16T15:01:52Z

4. Duplicate keys

go-yaml allows for duplicate keys goccy does not

There is a parser.Option for goccy to allow duplicate keys, parser.AllowDuplicateMapKey().

Mivr added 6 commits June 7, 2025 11:37

Initial implementation

1928e51

Initial implementation

9c71168

Clean up

4691c57

Clean up

a9e4376

Run operators tests with yaml and Goccy implementations

b1f9024

Fix linter errors and ignore tests where Goccy is different in parsing

0143654

Mivr changed the title ~~#2298/change yaml parser to a maintained one~~ WIP: #2298/change yaml parser to a maintained one Jun 7, 2025

Mivr changed the title ~~WIP: #2298/change yaml parser to a maintained one~~ Draft: #2298/change yaml parser to a maintained one Jun 7, 2025

Mivr marked this pull request as draft June 7, 2025 09:08

Mihail Vratchanski added 2 commits June 7, 2025 12:14

Remove parser comparison test, fix code after rebase

37fd857

Return debug statements

f3d302b

Mivr mentioned this pull request Jun 7, 2025

#2298 change yaml parser to a maintained one #2389

Closed

Fix tests

67572e3

ccoVeille reviewed Jun 7, 2025

View reviewed changes

Mivr added 6 commits June 8, 2025 18:00

Transition tests to skip variables, add Goccy specific tests in their…

c956c31

… place, add datetime preprocessor for date time difference

Change v3 argument to legacy-v3 for clarity

b7eaaa6

Add tests for goccy comments handling

8586622

Add tests showing the different behaviour of goccy for anchors

85b4ba6

Add tests showing the different behaviour of goccy for flow maps (yam…

962b14e

…l.v3 preserves them while goccy normalises them)

Add unsupported maps tests for Goccy

3857932

ccoVeille reviewed Jun 9, 2025

View reviewed changes

Mivr and others added 2 commits June 12, 2025 00:16

PR comment

d338705

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>

PR comment

99d3544

Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com>

PR comments

1300c7b

Mivr mentioned this pull request Jun 16, 2025

[FR]: pnpm 10 aspect-build/rules_js#2100

Closed

		var commentLineRegEx = regexp.MustCompile(`^\s*#`)
		var yamlDirectiveLineRegEx = regexp.MustCompile(`^\s*%YA`) // e.g., %YAML

Uh oh!

Draft: #2298/change yaml parser to a maintained one #2392

Are you sure you want to change the base?

Draft: #2298/change yaml parser to a maintained one #2392

Uh oh!

Conversation

Mivr commented Jun 7, 2025

Uh oh!

ccoVeille left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mivr Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mivr commented Jun 11, 2025

Uh oh!

mikefarah commented Jun 11, 2025

Uh oh!

Mivr commented Jun 16, 2025

Thesis

go-yaml vs goccy parsing differences (not mitigated already in this PR)

1. Comments association on array elements (BREAKING change if someone depends on comments inside the yaml file)

2. Flow maps will be expanded:

3. Merge anchor

4. Duplicate keys

5. Goccy is stricter on some invalid YAML cases

Conclusion

Uh oh!

Mivr commented Jun 16, 2025

Uh oh!

LarsStegman commented Jun 16, 2025

4. Duplicate keys

Uh oh!

Uh oh!

Mivr Jun 7, 2025 •

edited

Loading