Opened 15 years ago
Closed 9 years ago
#6668 closed Cleanup/optimization (fixed)
Optimise utils.text wrap
| Reported by: | Chris Beaven | Owned by: | Markus Amalthea Magnuson |
|---|---|---|---|
| Component: | Utilities | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
In my work on #6667 I spent some time optimising the django.utils.text wrap method.
Here's the patch.
Attachments (2)
Change History (20)
Changed 15 years ago by
| Attachment: | wrap_test.py added |
|---|
comment:1 Changed 15 years ago by
| Owner: | changed from nobody to Chris Beaven |
|---|---|
| Status: | new → assigned |
| Triage Stage: | Unreviewed → Design decision needed |
The test results:
Sentence old: 1.43731307983 new: 0.553267002106 Paragraph old: 4.15362501144 new: 1.33707404137 10 Paragraphs old: 17.3589639664 new: 5.5629529953
(I'm not sure if this really needs to go through design decision, but I'll put it there anyway)
comment:2 Changed 15 years ago by
On this and #6668, is there a good reason to avoid use of Python's own built-in "textwrap" module?
comment:4 Changed 15 years ago by
It could potentially be implemented, but it messes with whitespace. My patch was intended to be identical to the current one (it's actually just a side-effect of me writing the mail wrap funciton).
The built-in one definitely isn't good enough for #6667.
comment:5 Changed 12 years ago by
| Type: | → Cleanup/optimization |
|---|
comment:6 Changed 12 years ago by
| Severity: | → Normal |
|---|
comment:7 Changed 12 years ago by
| Easy pickings: | unset |
|---|---|
| Triage Stage: | Design decision needed → Accepted |
| UI/UX: | unset |
Accepted, see #6667.
comment:8 Changed 10 years ago by
| Component: | Core (Other) → Utilities |
|---|
comment:9 Changed 10 years ago by
| Patch needs improvement: | set |
|---|
The patch no longer applies, and the test must be rewritten as unittests.
comment:10 Changed 10 years ago by
I tested the patch that has the changes to text.py with the function wrap(). The original patch still works. The existing tests all pass.
comment:11 Changed 10 years ago by
| Owner: | changed from Chris Beaven to Susan Tan |
|---|
comment:12 Changed 10 years ago by
See PR: https://github.com/django/django/pull/1339 SmileyChris's patch works against existing utils_tests suite.
comment:13 Changed 10 years ago by
As noted on the PR, it looks like this introduces an off by one error in the wordcount filter. Failing test is test_wordcount (defaultfilters.tests.DefaultFiltersTests)
comment:14 Changed 10 years ago by
| Owner: | Susan Tan deleted |
|---|---|
| Status: | assigned → new |
comment:15 Changed 9 years ago by
| Owner: | set to Markus Amalthea Magnuson |
|---|---|
| Status: | new → assigned |
I've updated this patch to pass all tests, here's a new pull request:
https://github.com/django/django/pull/2668
The fix was to wrap the max_width = lines in min() to make sure it never exceeds the given wrap width.
comment:16 Changed 9 years ago by
| Patch needs improvement: | unset |
|---|
comment:17 Changed 9 years ago by
However, I have no idea why the wordwrap tests are in test_wordcount(). I added another commit to just move them to their own method.
comment:18 Changed 9 years ago by
| Resolution: | → fixed |
|---|---|
| Status: | assigned → closed |
Here's the test file I used (you'll have to copy my new method to
new_wrapto run it yourself)