Skip to content
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

Two-phase pipeline parsing #2238

Merged
merged 3 commits into from
Jul 24, 2023
Merged

Two-phase pipeline parsing #2238

merged 3 commits into from
Jul 24, 2023

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Jul 24, 2023

There's a bug where multiple map merges aren't unmarshaled into structs in the way we've supported them.

To address this we need to go back to having greater control over the YAML unmarshaling process. It will also help if we, say, want to switch YAML library later on - if a new library supports the things we need to do, then we can simplify, and if it doesn't (or has subtle differences) then the intermediate processing can be the place where we make up the gap.

The key change here is from allowing yaml.v3 to unmarshal into structs using its reflective logic to:

  • Use yaml.v3 to parse into *yaml.Node, which provides all the relevant information in the document without further processing.
  • Unmarshal that node using ordered, which handles merges and aliases (including multiple merges).
  • Unmarshal the dynamic, weakly-typed structure made up of *ordered.MapSA, []any, and basic types into each struct manually.

The last manual part could be converted into a yaml.v3-like or encoding/json-like reflection-based approach at some point.

Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

Amazing stuff!

internal/pipeline/parser_test.go Outdated Show resolved Hide resolved
internal/pipeline/parser_test.go Outdated Show resolved Hide resolved
DrJosh9000 and others added 2 commits July 24, 2023 15:12
Co-authored-by: Narthana Epa <narthana.epa@gmail.com>
Copy link
Contributor

@moskyb moskyb left a comment

Choose a reason for hiding this comment

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

fantastic work 🎉

@DrJosh9000 DrJosh9000 merged commit 2ddaa04 into main Jul 24, 2023
1 check passed
@DrJosh9000 DrJosh9000 deleted the merge-madness branch July 24, 2023 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants