rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

fix(deps): replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml #89

Merged
aweiker merged 13 commits from review-bot-issue-87 into main 2026-05-13 03:47:02 +00:00
Conversation 61 Commits 13 Files Changed 6
+1 -2
1 changed files with 1 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0b16c4143a - Show all commits
review/persona_test.go
+1 -2
View File
13
 @@ -490,8 +490,6 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
}
func TestYAMLEmptyFileRejection(t *testing.T) {
sonnet-review-bot commented 2026-05-12 21:08:09 +00:00
Review
Copy Link
Copy Source

[MINOR] The TestYAMLEmptyFileRejection test uses tc.name (e.g., "completely empty") as the filename fragment but writes all files to the same dir. The test case names contain spaces which are valid in filenames but could cause issues on some filesystems. More importantly, the test writes files with names like completely empty.yaml — using spaces in filenames is unusual and could cause subtle test issues. Prefer replacing spaces with underscores or hyphens.

**[MINOR]** The `TestYAMLEmptyFileRejection` test uses `tc.name` (e.g., `"completely empty"`) as the filename fragment but writes all files to the same `dir`. The test case names contain spaces which are valid in filenames but could cause issues on some filesystems. More importantly, the test writes files with names like `completely empty.yaml` — using spaces in filenames is unusual and could cause subtle test issues. Prefer replacing spaces with underscores or hyphens.
sonnet-review-bot commented 2026-05-13 00:40:32 +00:00
Review
Copy Link
Copy Source

[NIT] TestYAMLEmptyFileRejection has a redundant if err != nil guard on the second condition: if err != nil && !strings.Contains(...). Since the preceding if err == nil { t.Error(...); return } (implicit — actually the test doesn't return on the first error, it falls through) — actually the first check is if err == nil { t.Error(...) } without a return, so err could still be nil here. This means the strings.Contains check is guarded correctly with err != nil, but the test won't t.Error the wrong message if err == nil (it will have already errored on the first check). The pattern is slightly unusual compared to the rest of the file which uses early returns. Consistent style would be to add a return after the first t.Error, then drop the err != nil guard.

**[NIT]** `TestYAMLEmptyFileRejection` has a redundant `if err != nil` guard on the second condition: `if err != nil && !strings.Contains(...)`. Since the preceding `if err == nil { t.Error(...); return }` (implicit — actually the test doesn't return on the first error, it falls through) — actually the first check is `if err == nil { t.Error(...) }` without a return, so `err` could still be nil here. This means the `strings.Contains` check is guarded correctly with `err != nil`, but the test won't `t.Error` the wrong message if `err == nil` (it will have already errored on the first check). The pattern is slightly unusual compared to the rest of the file which uses early returns. Consistent style would be to add a `return` after the first `t.Error`, then drop the `err != nil` guard.
dir := t.TempDir()
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
tests := []struct {
name string
content string
@@ -503,6 +501,7 @@ func TestYAMLEmptyFileRejection(t *testing.T) {
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
path := filepath.Join(dir, tc.name+".yaml")
if err := os.WriteFile(path, []byte(tc.content), 0644); err != nil {
t.Fatalf("failed to write test file: %v", err)
1
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.
rodin commented 2026-05-12 22:23:19 +00:00
Review
Copy Link
Copy Source

Fixed in 0b16c41: moved t.TempDir() inside each subtest for proper isolation.

Fixed in 0b16c41: moved `t.TempDir()` inside each subtest for proper isolation.