Skip to content

Conversation

@shuheiktgw
Copy link
Collaborator

Closes #736

Before submitting your PR, please confirm the following.

  • Describe the purpose for which you created this PR.
  • Create test code that corresponds to the modification
@shuheiktgw shuheiktgw requested a review from Copilot May 25, 2025 14:15
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.86%. Comparing base (04f9bb5) to head (76313fb).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
+ Coverage   77.83%   77.86%   +0.03%     
==========================================
  Files          22       22              
  Lines        8056     8067      +11     
==========================================
+ Hits         6270     6281      +11     
  Misses       1370     1370              
  Partials      416      416              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
Copy link

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

Adds support for multi-line double-quoted YAML strings with CRLF line endings

  • Handles \r (and optional \n) in scanDoubleQuote to treat CRLF as a newline
  • Adds decoder tests for line continuations using LF, CRLF, and CR

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
scanner/scanner.go Handle \r in scanDoubleQuote, skip following \n in CRLF pairs
decode_test.go Add tests for multi-line continuations with LF, CRLF, and lone CR
s.progressLine(ctx)
progress = 1
// Skip \n after \r in CRLF sequences
if idx+2 < size && src[idx+2] == '\n' {
Copy link

Copilot AI May 25, 2025

Choose a reason for hiding this comment

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

Off-by-one check: when at a '\r' at src[idx], the '\n' to skip is at src[idx+1], not src[idx+2]. Update the condition to if idx+1 < size && src[idx+1] == '\n'.

Suggested change
if idx+2 < size && src[idx+2] == '\n' {
if idx+1 < size && src[idx+1] == '\n' {
Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not true. We have to check src[idx+2].

  • src[idx] = \
  • src[idx+1] = \r
  • src[idx+2] = \n
@shuheiktgw shuheiktgw marked this pull request as draft May 25, 2025 14:21
@shuheiktgw shuheiktgw marked this pull request as ready for review May 25, 2025 14:26
@goccy
Copy link
Owner

goccy commented May 29, 2025

LGTM 👍

@goccy goccy merged commit 680eea7 into master May 29, 2025
25 checks passed
@goccy goccy deleted the crlf-multi-line branch May 29, 2025 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants