This project is archived and is in readonly mode.
[PATCH] Rails 2.1.0 ActionView::Helpers::TextHelper#truncate
-
Michael Koziarski- Tag changed from 2.1.0, actionview, texthelper to 2.1.0, actionview, bugmash, texthelper
-
guilherme- Tag changed from 2.1.0, actionview, bugmash, texthelper to 2.1.0, 2.3.3, actionview, bugmash, texthelper
the truncate on my opinion doesn't need to fit the text on the length especified by user. so i made this patch.
i've checked the 2.3.3 version and still have this bug. this patch is for 2.3.3 :)
sorry for my last post.. i didn't expected to crash my text :(
-
truncate_fixed.diff
1.21 KB
-
guilherme
ops, it was for edge. !
-
jolohaga
I can confirm this bug.
Not only is the algorithm producing unpredictable results, but it uses :omission in the calculation. I agree with Guilherme that this doesn't seem the correct definition of truncate.
-
jolohaga
Unless I'm missing something, it seems to me that the documentation is murky. Cases in point:
Truncates a given +text+ after a given :length if +text+ is longer than :length
(defaults to 30). The last characters will be replaced with the :omission (defaults to "...").
#
==== Examples
#
truncate("Once upon a time in a world far far away")
# => Once upon a time in a world f...
"Once upon a time in a world far far away".length == 40 "Once upon a time in a world f...".length == 32
In any case, length is incorrect according to the documentation.
It is not equal to 33 (if we decide not to use :omission in the truncation algorithm)
and is not it equal to 30 (if we do decide to use :omission in the algorithm).#
truncate("Once upon a time in a world far far away", :length => 14)
# => Once upon a...
This example interprets the algorithm to include :omission, i.e. "Once upon a...".length == 14.
#
truncate("And they found that many people were sleeping better.", :length => 25, "(clipped)")
# => And they found that many (clipped)
This example interprets the algorithm to not include :omission. For example,
"And they found that many (clipped)".length == 34, "And they found that many ".length + "(clipped)".length = 25 + 9 = 34#
truncate("And they found that many people were sleeping better.", :omission => "... (continued)", :length => 15)
# => And they found... (continued)
This case interprets the default length overrides the user provided :length.
I think truncate should be simply truncate and nothing else. The :omission string should be appended to whatever the result of the truncation is. I also think whenever :length is greater than the string's length, the length of the truncation should be zero and the output is just the :omission string. There doesn't not need to be any magic in this simple method.
We need to come to a consensus and be clear in the documentation as to what truncation means.
-
jolohaga
Sorry for the headers. They resulted from copying documentation lines which were preceded by "#". If I could fix it I would. I'll know better next time.
-
Rizwan Rezaverified
+1 This is the behavior for truncate right now:
- I can verify that the length in the master branch is correct.
- The truncate method is taking omission into account when counting the length of the resulting string.
- The problem lies in omission. It's taking omission into account when I think omission should just be appended to the string after it has been truncated.
This behavior is happening both 2-3-stable and master.
-
Steve St. Martin- Title changed from Rails 2.1.0 ActionView::Helpers::TextHelper#truncate to [PATCH] Rails 2.1.0 ActionView::Helpers::TextHelper#truncate
+1 verified I've attached a patch that applies against 2.3.x and modified all tests to check for this behavior
-
truncate_fix.diff
4.63 KB
-
Dan Pickett+1 verified stevesmartin patch applies cleanly
-
Rizwan Reza
verified
-1 This patch only works in 2-3-stable. It would be great if this is also applicable to master.
-
Steve St. Martin
updated patch with documentation changes to reflect expected behavior
-
truncate_patch_2-3-stable.diff
1.57 MB
-
-
Steve St. Martin
I've attached a patch that will apply cleanly to master, updated tests and documentation as well
-
trucate_patch_master.diff
7.32 KB
-
-
Steve St. Martin
Ooops, maybe we should diff against 2-3-stable instead of master for a 2-3-stable only patch. Sorry :P
-
truncate_patch_2-3-stable.diff
6.38 KB
-
-
Dan Croak- Assigned user set to Pratik
+1 to stevestmartion's truncate_patch_2-3-stable.diff
It applies cleanly, rake test_action_pack runs green. Even covers 1.9 multibyte characters! Nice job.
-
Rizwan Reza
verified
+1 The new patch(es) apply cleanly.
-
Elise Huardverified
+1 expected behaviour. -
Rebecca Frankel
- Assigned user cleared.
verified
+1 The new patch apply cleanly to 2-3-stable the new code looked good to me
-
Greg Sterndale- Assigned user set to Pratik
+1 verified stevestmartin's truncate_patch_2-3-stable.diff patch applied, tests pass in 2-3-stable
nice job with examples in doc -
Jeremy KemperThese patches change truncate behavior - not ok for 2-3-stable.
Truncating to a specific length then appending the suffix is strange to me. Why would you expect this behavior? You're typically truncating to fit within an exact size.
-
Jeremy Kemper
- State changed from new to invalid
The truncation should definitely take the omission into account. The point is to restrict the truncated result to a specific size and not to exceed it.
-
Jeremy Kemper
- Tag changed from 2.1.0, 2.3.3, actionview, bugmash, texthelper to 2.1.0, 2.3.3, actionview, texthelper
-
jolohaga
@Jeremy
The documentation isn't consistent with this assumption. My botched (full of headers) comment above discusses this point. With better documentation there wouldn't be confusion.
By the way, appending omission seems reasonable to me (and obviously to others as well), but I respect core's decision.
-
Jeremy Kemper
- Assigned user changed from Pratik to Jeremy Kemper
- State changed from invalid to resolved
- Milestone changed from 2.x to 2.3.4
2-3-stable: 679a0bf17fa9d74a4475753e9075bed8aea59e9a
master: 618771beb57e623e05f70fd31f3bdb0c04318017