Skip to content

Conversation

ccoVeille
Copy link
Contributor

@ccoVeille ccoVeille commented Sep 19, 2025

This partially rolls back the changes made with 926dd0b with #47

The colon was allowed in the characters allowed for anchor and alias
name. But it leads to ambiguity in the YAML structure.

This PR also makes sure to emit a space when an alias is used in a key of a mapping node.

Related to:

The issue appeared with the refactoring made with #47 that made sure to respect the YAML specification that allows having a : in an anchor and alias name.

The issue was reported by @mikefarah when he tried to migrate on go.yaml.in/yaml/v4

@Copilot Copilot AI review requested due to automatic review settings September 19, 2025 22:11
Copilot

This comment was marked as outdated.

@ccoVeille ccoVeille marked this pull request as draft September 20, 2025 06:32
@ccoVeille
Copy link
Contributor Author

I should have mentionned it. This is draft. As a proof of concept and for discussion purpose.

@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from 851644f to 3d03825 Compare September 28, 2025 19:32
@ccoVeille ccoVeille marked this pull request as ready for review September 28, 2025 19:33
@ccoVeille
Copy link
Contributor Author

cc @mikefarah @inteon

@ingydotnet ingydotnet force-pushed the fix-emit-alias-in-mapping branch from 3d03825 to d98cd1a Compare September 29, 2025 18:45
@ingydotnet
Copy link
Member

I would prefer that we revert allowing : in anchors.
The libyaml family has always been overly strict about chars allowed in anchors.
They only allow [-\w] chars.
We allowed more chars (all the 7bit ascii allowed chars) but we are currently far from spec compliant.
The spec allows all unicode chars minus the syntax chars it disallows.
I feel this is both unimportant and potentially dangerous.
Removing : will not bother anyone and will cause less problems (like the one yq reported).

@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from d98cd1a to 7bb6299 Compare September 29, 2025 20:24
@ccoVeille ccoVeille requested a review from Copilot September 29, 2025 20:28
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

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


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

@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from 7bb6299 to ade13e8 Compare September 29, 2025 20:33
The DeepEqual asserter was too simple to cover this.
This partially rolls back the changes made with 926dd0b

The colon was allowed in the characters allowed for anchor and alias
name. But it leads to ambiguity in the YAML structure.
The idea is to make sure that a YAML alias in a mapping is followed by
a space before the :

An issue appeared with the refactoring made recently that made sure
to respect the YAML specification that allows having a `:` in an anchor
and alias name.
@ccoVeille ccoVeille force-pushed the fix-emit-alias-in-mapping branch from ade13e8 to 8d9ab3d Compare October 2, 2025 02:36
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