Skip to content

Conversation

@alexpasmantier
Copy link

This adds support for crlf sequences (carriage return + line feed) which weren't handled by the current implementation.

It also removes checks for a newline inside each line (which we make sure beforehand is free of any newlines anyway).

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

I see that this modifies 4 methods line, line_fast, span, span_fast. But there's only one unit test change at a sort of higher level which deals at the line level. Can you please take a look at whether there's some unit tests that need to be added to cover this change?

I think there's probably some docs that could be updated to to capture the newline handling behavior. Would you mind updating those too?

@joshka
Copy link
Member

joshka commented Mar 3, 2025

(also - apologies for not getting you a response on this sooner. It's not currently added to our automatic notifications in discord so I accidentally wasn't following the repo)

Cretezy added a commit to Cretezy/ansi-to-tui that referenced this pull request Mar 5, 2025
@Cretezy
Copy link
Contributor

Cretezy commented Mar 5, 2025

@joshka Looking at the code, it seems like span/span_fast previously checked for \n which can't be fed into it (since line/line_fast splits on \n, and now \r\n), so there's not much to test there.

@alexpasmantier
Copy link
Author

alexpasmantier commented Apr 14, 2025

@joshka no problem, and yes as @Cretezy mentioned, this is mainly:

  • cleaning redundant logic in sub-functions which are still passing the test cases
  • adding handling of crlf sequences at the "top-level" parser lines (which then orchestrates other parsers below which never see these) and testing that.

I believe this is an important feature/fix since these crlf sequences are not that rare and currently "break" the conversion from ansi to ratatui in all projects depending on this one.

PS: this should also marginally improve performance, which is always nice to take.

@alexpasmantier
Copy link
Author

@joshka added a couple of unit tests for the line and line_fast methods. I also don't really see any documentation worth updating in the repo.

Is there anything else that might prevent this from being merged?

@alexpasmantier alexpasmantier requested a review from joshka April 28, 2025 17:33
@joshka
Copy link
Member

joshka commented May 14, 2025

I just wanted to let you know that I haven't forgotten this one. I've just been traveling a bit the last month or so. I know that digging into how this one works requires a bit more time and effort than I've had available. I'll take another look soon.

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.

3 participants