Skip to content

Improve CRLF handling in the decoder#20

Open
smortezah wants to merge 3 commits intotoon-format:mainfrom
smortezah:fix/crlf
Open

Improve CRLF handling in the decoder#20
smortezah wants to merge 3 commits intotoon-format:mainfrom
smortezah:fix/crlf

Conversation

@smortezah
Copy link
Contributor

This pull request improves cross-platform compatibility in the toon_format scanner and decoder by normalizing different line endings (Windows CRLF and old Mac CR) to LF, ensuring consistent parsing behavior regardless of the source file's origin. It also adds comprehensive tests to verify correct handling of various line ending scenarios.

Line ending normalization:

  • Updated src/toon_format/_scanner.py to normalize Windows CRLF (\r\n) and old Mac CR (\r) line endings to LF (\n) before parsing, preventing stray carriage return characters from appearing in parsed content.

Test coverage improvements:

  • Added TestCRLFDecoding in tests/test_decoder.py to verify that the decoder correctly handles CRLF, CR, mixed line endings, and quoted strings containing escaped \r and \n sequences, including strict mode decoding.
  • Added TestCRLFHandling in tests/test_scanner.py to ensure the scanner normalizes all types of line endings, preserves indentation, and works correctly in strict mode.

@smortezah smortezah requested a review from a team as a code owner November 6, 2025 11:48
@Justar96
Copy link
Contributor

Justar96 commented Nov 6, 2025

Hi @smortezah,
Thanks for the pull request. Could you please resend it with the PR form? [](https://github.com/toon-format/toon-python/blob/main/.github/PULL_REQUEST_TEMPLATE.md) same with #21

@Justar96 Justar96 requested a review from Copilot November 8, 2025 19:46
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 enhances cross-platform compatibility by normalizing different line ending formats (CRLF, CR, LF) to a consistent LF format before parsing, preventing issues with files created on different operating systems.

Key Changes:

  • Added line ending normalization logic in the scanner to handle Windows (CRLF) and old Mac (CR) formats
  • Implemented comprehensive test coverage for CRLF/CR handling in both scanner and decoder
  • Ensured escaped sequences in quoted strings are preserved during normalization

Reviewed Changes

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

File Description
src/toon_format/_scanner.py Added line ending normalization logic to convert CRLF and CR to LF
tests/test_scanner.py Added TestCRLFHandling class with tests for various line ending scenarios
tests/test_decoder.py Added TestCRLFDecoding class to verify decoder handles different line endings correctly

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

bpradana
bpradana previously approved these changes Nov 9, 2025
Copy link

@bpradana bpradana left a comment

Choose a reason for hiding this comment

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

approved despite CI failed on lint
🚀🚀🚀

Updating comments for specificity of replacing characters

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@johannschopplich
Copy link
Contributor

Hey @smortezah – I've been going through the open PRs and yours stand out. The CRLF normalization, numeric validation fix, and pathlib support are all well-scoped, properly tested, and ready to go. Exactly the kind of work this project needs more of.

I just opened a discussion about bringing in new contributors with merge access: #52

If you're interested in being more involved, I'd love to have you.

@smortezah
Copy link
Contributor Author

@johannschopplich I'll be glad to help.

@smortezah
Copy link
Contributor Author

There is an issue with the lint GitHub action, that makes one of the checks here fail.

I've made the PR #53 to fix it.

@johannschopplich
Copy link
Contributor

@alesanfra this one's been sitting for a while and looks solid to me – happy if you want to take a quick look and merge if it checks out! 🙌

@alesanfra alesanfra self-requested a review March 17, 2026 08:43
Copy link

@alesanfra alesanfra left a comment

Choose a reason for hiding this comment

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

LGTM but some checks are failing, @smortezah can you please look into it? Thanks

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants