Skip to content
This repository has been archived by the owner on Aug 17, 2024. It is now read-only.

replaced split_line with a multibyte aware version #61

Merged
merged 5 commits into from
Mar 1, 2024
Merged

replaced split_line with a multibyte aware version #61

merged 5 commits into from
Mar 1, 2024

Conversation

ronnybremer
Copy link
Contributor

split_line was working fine as long as the input was just plain US-ASCII. When using multibyte characters and they happened to be at the end of the line to spit the function did panic.

The new code returns correctly split lines even if they contain multibyte characters.

split_line was working fine as long as the input was just plain US-ASCII. When using multibyte characters and they happened to be at the end of the line to spit the function did panic.

The new code returns correctly split lines even if they contain multibyte characters.
@ronnybremer
Copy link
Contributor Author

related to #60

@Peltoche
Copy link
Owner

Hello @ronnybremer, thanks for the PR. I will take a look asap

Copy link
Owner

@Peltoche Peltoche left a comment

Choose a reason for hiding this comment

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

Coule you add a test case with your exemple from #60 please? To ensure that will not recreate the bug.

@ronnybremer
Copy link
Contributor Author

Coule you add a test case with your exemple from #60 please? To ensure that will not recreate the bug.

Done. I also fixed the broken test from before, needed to take 75 characters of the first line but only 74 characters of subsequent lines as a " " is added in front of each line.

the text to test should include the expected newline characters
@ronnybremer
Copy link
Contributor Author

I had to fix the test, too bad I can't run it myself without checking out the repo. The text variable needs to include the expected line breaks, which I now added. Hopefully it works now. Sorry for the hassle.

@ronnybremer
Copy link
Contributor Author

@Peltoche would you please start the testing stage again? Thank you.

@ronnybremer
Copy link
Contributor Author

one last wrong character, can't believe it :(

@Peltoche
Copy link
Owner

Peltoche commented Mar 1, 2024

Sorry for the delay.

@Peltoche
Copy link
Owner

Peltoche commented Mar 1, 2024

LGTM, thanks for this PR 👍

@Peltoche Peltoche merged commit e435769 into Peltoche:master Mar 1, 2024
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants