Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trim whitespace from EOLs when line split is found #395

Merged
merged 4 commits into from
Mar 19, 2024
Merged

trim whitespace from EOLs when line split is found #395

merged 4 commits into from
Mar 19, 2024

Conversation

batwomankt
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

In SplitAt, when a TextLine is split at exact length match - the loop will no longer grab and trim the whitespace on the next iteration because it exits early (Issue 367 fix). Instead, added a whitespace trim in SplitAt when the exact length match is being examined.

See Issue 393 for a visual representation. More detail can be provided upon request.

@@ -1328,6 +1328,9 @@ public TextLine SplitAt(LineBreak lineBreak, bool keepAll)

if (index == 0)
{
// Now trim trailing whitespace from this line in the case of an exact
// length line break (non CJK)
this.TrimTrailingWhitespaceAndRecalculateMetrics();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that in case of exact length line breaks, there could be whitespace that remains untrimmed - the addition of this line here would take care of that (without losing the benefits of the fix added for issue 367)

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

This all looks great. Thanks for the contribution!

EDIT.

Looks like the tests still fail?

@batwomankt
Copy link
Contributor Author

batwomankt commented Mar 19, 2024

This all looks great. Thanks for the contribution!

EDIT.

Looks like the tests still fail?

Yeah, I had missed one final spot above where whitespace needed the extra trimming!

@JimBobSquarePants JimBobSquarePants merged commit 30c8f15 into SixLabors:main Mar 19, 2024
8 checks passed
@batwomankt batwomankt deleted the fix-issue-393 branch March 19, 2024 12:15
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.

2 participants