Skip to content

Conversation

inteon
Copy link
Contributor

@inteon inteon commented Aug 31, 2025

depends on #95

Apply the missing changes from upstream libyaml (see commit messages for individual changes). Additionally, in c47598a, I fixed the yts tests that started failing (on master, they were passing for the wrong reasons).

Upstream commits:

@Copilot Copilot AI review requested due to automatic review settings August 31, 2025 15:21
Copilot

This comment was marked as outdated.

@inteon inteon marked this pull request as draft August 31, 2025 15:30
@ccoVeille
Copy link
Contributor

Apply the missing changes from upstream libyaml (see commit messages for individual changes). Additionally, in c47598a, I fixed the yts tests that started failing (on master, they were passing for the wrong reasons).

Impressive, thank you so much.

@inteon inteon force-pushed the apply_upstream branch 3 times, most recently from 2f1b2ad to 0b2698f Compare September 2, 2025 19:11
@inteon inteon requested a review from Copilot September 2, 2025 19:11
@inteon inteon marked this pull request as ready for review September 2, 2025 19:11
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR applies upstream changes from libyaml to the Go YAML library, incorporating multiple bug fixes and feature updates from the C library. The changes primarily focus on improving YAML parsing, validation, and emission behavior to align with upstream standards.

Key changes include:

  • Added support for YAML version 1.2 directives alongside the existing 1.1 support
  • Fixed parser state management for flow sequences and document handling
  • Updated scanner logic for tag URIs and plain scalars to be more restrictive and correct

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
yts/known-failing-tests Removed test cases that now pass due to upstream fixes
yamlh.go Added version directive tracking and end type enumeration for better document state management
scannerc.go Updated tag URI scanning logic and fixed plain scalar parsing restrictions
parserc.go Enhanced version directive support and improved flow sequence parsing
encode.go Updated to use new end type enumeration
emitterc.go Comprehensive updates to document emission logic and YAML version handling

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ingydotnet
Copy link
Member

ingydotnet commented Sep 2, 2025

@inteon Thanks for this. I amended each of your commits (and pushed them back to you) to add URLs pointing back to the libyaml commits to make it easier for people to review.

I still need to review.

@ingydotnet ingydotnet self-requested a review September 2, 2025 21:27
@ingydotnet
Copy link
Member

FWIW, go-yaml was ported from libyaml which was ported from pyyaml.
All of these are under the github.com/yaml org.

A goal of the org and this project is to see them all come into sync.
Also to identify all the most popular ports out there and to add supported forks to the yaml org, or submit patches to them.

It would be good to have tools to identify the current state of these things.

@dolmen
Copy link

dolmen commented Sep 3, 2025

I can't review all at once: comparing each Go commit to libyaml is an important effort. I would prefer separate PR for each.
So I'll skip that one.

@ingydotnet
Copy link
Member

I can't review all at once: comparing each Go commit to libyaml is an important effort. I would prefer separate PR for each. So I'll skip that one.

This is fair.

@inteon Would you mind turning one or two of these commits into their own PR with one commit for each?

You could do all of them but I don't want to waste your time if there ends up being a problem with all of them.

Also could you copy the original merged libyaml PR text into the new PR? I know we have links to them, but probably better to have the text right there...

Love this work btw!

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.

4 participants