CLI refactoring #168

ingydotnet · 2025-11-03T16:41:52Z

Heavy refactoring and feature enhancement for the go-yaml CLI.

Adds a test suite for the CLI.

Note: Claude Code was used for much of this work so. Should be interesting to review.

Copilot

Pull Request Overview

This PR adds support for COMMENT tokens in the go-yaml CLI tool and exports Comment struct fields in the internal libyaml package. The changes enable the CLI to emit explicit COMMENT tokens in token output mode, provide better position formatting, and support both compact and profuse node/event modes.

Key changes:

Export Comment struct fields (Head, Line, Foot, ScanMark, TokenMark, StartMark, EndMark) from private to public
Add COMMENT_TOKEN type and implement comment token generation in the CLI parser
Implement compact node representation format and improve position formatting (from semicolon to slash separator)
Add comprehensive test infrastructure with test data files

Reviewed Changes

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

Show a summary per file

File	Description
internal/libyaml/yaml.go	Exports Comment struct fields and adds COMMENT_TOKEN type
internal/libyaml/scanner.go	Updates Comment field references to use exported names and adds EndMark to scanLineComment
internal/libyaml/parser.go	Updates Comment field references to use exported names
internal/libyaml/api.go	Adds GetPendingComments() and GetCommentsHead() methods for CLI access
cmd/go-yaml/parser.go	Implements comment token generation with processComments method
cmd/go-yaml/token.go	Adds CommentType field and updates field names to lowercase, implements COMMENT token formatting
cmd/go-yaml/node.go	Adds MapSlice type for key ordering, implements FormatNodeCompact, updates formatStyle and formatTag
cmd/go-yaml/main.go	Adds -N/--NODE flag support and multi-document handling
cmd/go-yaml/event.go	Implements implicit/explicit document markers, adds position format improvements
cmd/go-yaml/cli_test.go	New test infrastructure for CLI testing
cmd/go-yaml/testdata/*.yaml	New test data files for various CLI modes
GNUmakefile	Adds test-cli target and reorganizes test commands

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cmd/go-yaml/parser.go

internal/libyaml/scanner.go

Copilot · 2025-11-03T16:53:00Z

@ingydotnet I've opened a new pull request, #169, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull Request Overview

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

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cmd/go-yaml/node.go

Copilot · 2025-11-03T18:22:49Z

cmd/go-yaml/event.go

+func adjustColumn(col int) int {
+	if col > 0 {
+		return col - 1
+	}
+	return 0
+}


This function lacks documentation explaining why column adjustment is necessary. Add a comment clarifying that yaml.Node uses 1-based column indexing while libyaml uses 0-based indexing, and this function converts between the two.

for @ingydotnet

Copilot · 2025-11-03T18:22:49Z

cmd/go-yaml/event.go

+		startCol := adjustColumn(node.Column)
 		endLine := node.Line
-		endColumn := node.Column
+		endColumn := startCol
 		if node.Value != "" {
 			// For single-line values, add the length to the column
 			if !strings.Contains(node.Value, "\n") {
-				endColumn += len(node.Value)
-			} else {
-				// For multi-line values, we'd need more complex logic
-				// For now, just use the start position
-				endColumn = node.Column
+				endColumn = startCol + len(node.Value)
 			}
 		}


The end column calculation uses byte length instead of rune count. For multi-byte UTF-8 characters, this will produce incorrect column positions. Use utf8.RuneCountInString(node.Value) instead of len(node.Value) to correctly handle Unicode characters.

for @ingydotnet

Copilot · 2025-11-03T18:22:49Z

cmd/go-yaml/main.go

+			var buf bytes.Buffer
+			enc := yaml.NewEncoder(&buf)
+			enc.SetIndent(2)
+			enc.CompactSeqIndent()


The encoder's CompactSeqIndent() method is called without checking if it exists or handling potential errors. Verify this is a valid method on yaml.Encoder. If it's a custom extension, ensure it's properly available in the imported yaml package version.

Suggested change

enc.CompactSeqIndent()

cmd/go-yaml/node.go

ccoVeille

I'm doubtful.

The complexity of the code in go-yaml binary makes me think it's hard to maintain it

GNUmakefile

ccoVeille · 2025-11-03T18:57:21Z

cmd/go-yaml/cli_test.go

+	// Build the CLI binary if it doesn't exist
+	binaryPath := "../../go-yaml"
+	if _, err := os.Stat(binaryPath); os.IsNotExist(err) {
+		t.Logf("Building go-yaml binary...")
+		cmd := exec.Command("go", "build", "-o", binaryPath, ".")
+		if output, err := cmd.CombinedOutput(); err != nil {
+			t.Fatalf("Failed to build go-yaml: %v\n%s", err, output)
+		}
+	}


It's not logic to build it in Go.

go run can be used

Also, code should be written in a way go test could be enough to emulate everything

The usual and idiomatic way in Go is to use os.ExecPipe and to play with it to pass data to Stdin and read result of Stdout

Using exec and string.Buffer is not the way to run and test go code. It would be of the binary launched was a blackbox binary

for @SVilgelm

ccoVeille · 2025-11-03T19:06:54Z

cmd/go-yaml/parser.go

+
+// appendCommentTokenIfNotEmpty creates and appends a comment token if the value is not empty.
+func (p *Parser) appendCommentTokenIfNotEmpty(value []byte, commentType string, comment *libyaml.Comment) {
+	if len(value) > 0 {


Suggested change

if len(value) > 0 {

if len(value) == 0 {

return

}

for @SVilgelm

cmd/go-yaml/node.go

ccoVeille · 2025-11-03T19:13:29Z

cmd/go-yaml/node.go

+		if tag := formatTag(n.Tag, n.Style, false); tag != "" && tag != "!!str" {
+			result = append(result, MapItem{Key: "tag", Value: tag})
+		}
+		if n.Anchor != "" {
+			result = append(result, MapItem{Key: "anchor", Value: n.Anchor})
+		}
+		if n.HeadComment != "" {
+			result = append(result, MapItem{Key: "head", Value: n.HeadComment})
+		}
+		if n.LineComment != "" {
+			result = append(result, MapItem{Key: "line", Value: n.LineComment})
+		}
+		if n.FootComment != "" {
+			result = append(result, MapItem{Key: "foot", Value: n.FootComment})


!!str, tag, head, foot, …

All of them could be constants

It would avoid possible errors such as typo like "stt", "hesd"

for @SVilgelm

1. Use periods instead of semicolons: 1;2 → 1/2 2. Shorten same-line ranges: 1;2-1;5 → 1/2-5 (omit repeated line number) 3. Keep full format for cross-line ranges: 1;2-3;5 → 1/2-3/5

Co-authored-by: Copilot <[email protected]>

...for consistency Co-authored-by: ingydotnet <[email protected]>

Co-authored-by: Copilot <[email protected]>

ccoVeille · 2025-11-03T20:54:58Z

GNUmakefile

+	go test$(if $v, -v)

 test-yts: $(GO-DEPS) $(YTS-DIR)
 	go test$(if $v, -v) ./yts -count=$(count)


Suggested change

go test$(if $v, -v) ./yts -count=$(count)

go test$(if $v, -v) ./yts/... -count=$(count)

ccoVeille · 2025-11-03T20:57:13Z

GNUmakefile


 # Test rules:
-test: $(GO-DEPS)
-	go test$(if $v, -v) -vet=off ./...


You are removing the vet=off we just added 😁 also the ./...

ccoVeille · 2025-11-03T20:57:43Z

GNUmakefile

 	@RUNFAILING=1 bash -c "$$yts_pass_fail"

+test-cli: $(GO-DEPS) cli
+	go test$(if $v, -v) ./cmd/go-yaml -count=$(count)


Suggested change

go test$(if $v, -v) ./cmd/go-yaml -count=$(count)

go test$(if $v, -v) ./cmd/go-yaml/... -count=$(count)

ccoVeille · 2025-11-03T20:58:46Z

GNUmakefile

-fmt: $(GOLANGCI-LINT-VERSIONED)
-	$< fmt ./...
-


There is no reason to remove make fmt

ingydotnet requested review from SVilgelm, ccoVeille, Copilot and stefanprodan November 3, 2025 16:41

Copilot AI reviewed Nov 3, 2025

View reviewed changes

cmd/go-yaml/parser.go Outdated Show resolved Hide resolved

internal/libyaml/scanner.go Show resolved Hide resolved

Copilot AI mentioned this pull request Nov 3, 2025

Add ScanMark to scanLineComment for consistency #169

Closed

ingydotnet requested a review from Copilot November 3, 2025 18:20

Copilot AI reviewed Nov 3, 2025

View reviewed changes

ccoVeille reviewed Nov 3, 2025

View reviewed changes

ingydotnet and others added 16 commits November 3, 2025 15:20

Have CLI report comments as tokens

ce220d1

Use lowercase keys for token and event info

b29b7e5

Changed the position format in go-yaml CLI

d9b8a5c

1. Use periods instead of semicolons: 1;2 → 1/2 2. Shorten same-line ranges: 1;2-1;5 → 1/2-5 (omit repeated line number) 3. Keep full format for cross-line ranges: 1;2-3;5 → 1/2-3/5

Add a YAML data driven test system for CLI

60b5d9c

Add 'go-yaml -N' support for more verbose nodes

6430df7

Make CLI -n and -N output more compact

d7fbcd3

Make -n --node output more compact

e92c3f0

Use literal scalars for all data points in tests

437e74d

Set specific key order for 'go-yaml -n' output

528d58e

Show -n multi doc input as sequence output

97ef073

Add CLI support for 'implicit' on doc start event

54c06c4

Fix some linting errors

234fc0d

Add 'make check' support to run all checks

a0e5432

Refactor to remove some duplication

895e4a2

Co-authored-by: Copilot <[email protected]>

Add ScanMark field to Comment in scanLineComment

bf504c4

...for consistency Co-authored-by: ingydotnet <[email protected]>

Some makefile conditioning

7747504

ingydotnet force-pushed the cli-refactoring branch from 5e80bc3 to 7747504 Compare November 3, 2025 20:32

Update cmd/go-yaml/node.go

49541c4

Co-authored-by: Copilot <[email protected]>

ccoVeille reviewed Nov 3, 2025

View reviewed changes

	go test$(if $v, -v) ./yts -count=$(count)
	go test$(if $v, -v) ./yts/... -count=$(count)

	go test$(if $v, -v) ./cmd/go-yaml -count=$(count)
	go test$(if $v, -v) ./cmd/go-yaml/... -count=$(count)

CLI refactoring #168

Are you sure you want to change the base?

CLI refactoring #168

Uh oh!

Conversation

ingydotnet commented Nov 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Nov 3, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ccoVeille left a comment

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants