Skip to content

Conversation

@shuheiktgw
Copy link
Collaborator

@shuheiktgw shuheiktgw commented Jun 2, 2025

Closes #748

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 June 2, 2025 06:44
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

This PR addresses the normalization of multi-line strings by converting CR and CRLF to LF, ensuring consistent line breaks.

  • Updated the multi-line scanning in scanner/scanner.go to properly handle CR and CRLF sequences.
  • Expanded decode_test.go with test cases covering inputs with CR, CRLF, and LF, verifying normalized outputs.

Reviewed Changes

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

File Description
scanner/scanner.go Implements CR and CRLF normalization in multi-line strings.
decode_test.go Adds tests for verifying the normalization of line breaks.
Comments suppressed due to low confidence (2)

scanner/scanner.go:1160

  • [nitpick] Consider renaming the variable 'crlf' to 'foundCRLF' for improved clarity on its purpose.
var crlf bool

scanner/scanner.go:1174

  • Add an inline comment explaining why subtracting 1 from endPos is necessary when a CRLF is detected, to assist future maintainers in understanding this adjustment.
if crlf { endPos = endPos - 1 }
@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 78.02%. Comparing base (52dacb8) to head (ac837bc).

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
+ Coverage   77.97%   78.02%   +0.04%     
==========================================
  Files          22       22              
  Lines        8108     8126      +18     
==========================================
+ Hits         6322     6340      +18     
  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.
s.offset++
}
c = '\n'
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not fully confident about this change, since most of the logic is quite similar to what's in the scanNewLine method. I tried to refactor and deduplicate it, but couldn’t come up with a better approach. Open to any suggestions 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants