-
Notifications
You must be signed in to change notification settings - Fork 79
Trim trailing whitespace in TextElements #247
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 trailing whitespace in TextElements #247
Conversation
5d4719e
to
20cfdae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a nit about the .
.
But really, I'm not sure trimRight
is right.
Can you add a test for nbsp
or zwnj
?
Also, if it is, use trimEnd
, as that's what's standardized?
# < whitespace > | ||
key1 = Value | ||
|
||
key2 = Value {placeable}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you didn't mean to have the trailing .
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doch doch. I only meant to test that TextElements
not in the last position are not affected by the trimming logic.
Ah, I see what you mean. Let me see.
It's only available in Firefox 61+ and Chrome 66+, and not in node. I don't want to get bitten by its being missing in a |
// non-empty line will decide what to do with it. | ||
const firstLineContent = start !== eol | ||
// Trim the trailing whitespace in case this is a single-line pattern. | ||
// Multiline patterns are parsed anew by getComplexPattern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to one day be able to pass first line to complex string and go from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's doable, but keep in mind that we'd need to parse firstLineContent
in getComplexPattern
anyways in case it contains placeables or escape sequences. The current approach of rewinding the index to the beginning of the first line works pretty well IMO.
No description provided.