Conversation
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #740 +/- ##
=======================================
Coverage 77.94% 77.94%
=======================================
Files 22 22
Lines 7998 7998
=======================================
Hits 6234 6234
Misses 1349 1349
Partials 415 415 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR updates YAML quoting behavior so that the backslash character no longer requires a quoted string value, easing transition from goyaml for system-dependent paths.
- Updated IsNeedQuoted logic in token/token.go to remove backslash as a trigger for quoting.
- Adjusted tests in token/token_test.go, encode_test.go, and decode_test.go to reflect the new expectations for YAML output and parsing of backslashes.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| token/token_test.go | Removed a test case for a quoted backslash value. |
| token/token.go | Updated quoting logic by removing the backslash check in the switch. |
| encode_test.go | Modified expected YAML output for backslash-containing strings and removed an obsolete test case. |
| decode_test.go | Added a test to verify that Windows file paths with backslashes are correctly parsed. |
Comments suppressed due to low confidence (3)
encode_test.go:366
- Verify that the new expected output for unquoted backslashes is consistent in different edge cases; consider expanding tests to cover any similar scenarios.
a: \0
encode_test.go:781
- If the removal of this single-quoted test case is intentional given the new quoting rules, consider adding new tests that cover these scenarios using unquoted values.
a: '\.yaml'
decode_test.go:3730
- The addition of this Windows path test is beneficial; you might also add tests for other operating system path formats to ensure broad coverage.
test:
| for i, c := range value { | ||
| switch c { | ||
| case '#', '\\': | ||
| case '#': |
There was a problem hiding this comment.
Consider adding a comment here to explain why the backslash no longer triggers the need to quote the string, clarifying that this change supports system-dependent paths.
This PR has been open since May 20 but it resolves the multi-line issues: goccy/go-yaml#740 goccy/go-yaml#801 goccy/go-yaml#802 Forked the repo and merged that branch into my fork in order to resolve the issue.
Use of character
\in a string doesn't require yaml value to be quotedNote: This makes transition from goyaml easier for yaml file with system-dependent paths