Replace rstrip with chomp!(break_sequence) in word_wrap

[maxim]

Summary

When using ActionView#word_wrap with non-whitespace break sequence, it doesn't strip the ending sequence properly.

# <%= word_wrap("11 22 33", line_width: 2, break_sequence: "\n# ") %>

expected

# 11
# 22
# 33

actual

# 11
# 22
# 33
#

I replaced .rstrip with .chomp!(break_sequence) to fix this.

[jonathanhefner]

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

[jonathanhefner]

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:

1. (\s+|$) can always match the end of a string. (A string will either end with whitespace or not.)
2. (.{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.
3. 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.
4. If the first match of (.{1,#{line_width}})(\s+|$) is at the end of a string, then gsub will append break_sequence as necessary.
5. 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.

[maxim]

@jonathanhefner

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?

[maxim]

I amended the commit with a CHANGELOG entry. 👍🏻 Let me know if I should fix anything with its wording.

[jonathanhefner]

Looks good! Thank you, @maxim! 😃