-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Replace rstrip with chomp!(break_sequence) in word_wrap #45942
Conversation
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.
The break_sequence
option was added in #21086, and it looks like the current behavior is an oversight, so I think fixing it makes sense. 👍
We should probably add a CHANGELOG entry though, with an example of the difference in behavior. It might be good to include a line break in the example input too. e.g.:
# <%= word_wrap("11 22\n33 44", line_width: 2, break_sequence: "\n# ") %>
Before:
# 11
# 22
#
# 33
# 44
#
After:
# 11
# 22
# 33
# 44
@@ -263,7 +263,7 @@ def pluralize(count, singular, plural_arg = nil, plural: plural_arg, locale: I18 | |||
# # => Once\r\nupon\r\na\r\ntime | |||
def word_wrap(text, line_width: 80, break_sequence: "\n") | |||
text.split("\n").collect! do |line| | |||
line.length > line_width ? line.gsub(/(.{1,#{line_width}})(\s+|$)/, "\\1#{break_sequence}").rstrip : line | |||
line.length > line_width ? line.gsub(/(.{1,#{line_width}})(\s+|$)/, "\\1#{break_sequence}").chomp!(break_sequence) : line |
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.
This does change the behavior slightly when extra whitespace is at the end of a line:
# Before
word_wrap("1234 123 \n1234", line_width: 4)
# => "1234\n123\n1234"
# After
word_wrap("1234 123 \n1234", line_width: 4)
# => "1234\n123 \n1234"
Although that arguably makes it more consistent with how extra intra-line whitespace is treated when wrapping:
# Before
word_wrap("123 123 \n1234", line_width: 4)
# => "123 \n123\n1234"
# After
word_wrap("123 123 \n1234", line_width: 4)
# => "123 \n123 \n1234"
(That said, my own personal preference would be to consistently strip every line, e.g. "123\n123\n1234"
.)
Also, for posterity:
chomp!
returns nil
when the string does not end with the given argument. However, the string returned by gsub
will always end with break_sequence
because:
(\s+|$)
can always match the end of a string. (A string will either end with whitespace or not.)(.{1,#{line_width}})(\s+|$)
can always match a character (either non-whitespace or whitespace) followed by any number of whitespace at the end of a string.line.length > line_width
guarantees there will be at least 1 character in the string, so(.{1,#{line_width}})(\s+|$)
will always match at least once.- If the first match of
(.{1,#{line_width}})(\s+|$)
is at the end of a string, thengsub
will appendbreak_sequence
as necessary. - If the first match of
(.{1,#{line_width}})(\s+|$)
is not at the end of a string, then there will be a trailing substring with at least 1 character. Go to 3.
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.
my own personal preference would be to consistently strip every line
I could definitely see the argument for convenience. I just worry that by going this route we make it harder to reason about word_wrap
's behavior, or apply it in situations where each character matters. And it helps that Ruby provides great tools such as String#squeeze
to preprocess a string prior to wrapping.
Also, for posterity:
[…]
Thanks for writing these out! 🙏🏻 A super important set of happy accidents. I was also worried about nil
, and did a bunch of sanity checks prior to PRing, but I'm sure a developer could easily miss this and introduce a bug. Perhaps it'd make sense to add a comment like: "chomp! requires that this string always ends with break_sequence". What do you think?
When word_wrap's break_sequence contains non-rstrippable characters, word_wrap fails to strip the extra break_sequence at the end properly. Using chomp! helps cut exactly what was specified in the argument.
d405916
to
fe554a3
Compare
I amended the commit with a CHANGELOG entry. 👍🏻 Let me know if I should fix anything with its wording. |
Looks good! Thank you, @maxim! 😃 |
Summary
When using
ActionView#word_wrap
with non-whitespace break sequence, it doesn't strip the ending sequence properly.expected
actual
I replaced
.rstrip
with.chomp!(break_sequence)
to fix this.