Opened 17 years ago

Closed 10 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)

wrap_test.py (570 bytes ) - added by Chris Beaven 17 years ago.
Here's the test file I used (you'll have to copy my new method to new_wrap to run it yourself)
faster_wrap.diff (3.3 KB ) - added by Chris Beaven 17 years ago.
and here's the patch

Download all attachments as: .zip

Change History (20)

by Chris Beaven, 17 years ago

Attachment: wrap_test.py added

Here's the test file I used (you'll have to copy my new method to new_wrap to run it yourself)

by Chris Beaven, 17 years ago

Attachment: faster_wrap.diff added

and here's the patch

comment:1 by Chris Beaven, 17 years ago

Owner: changed from nobody to Chris Beaven
Status: newassigned
Triage Stage: UnreviewedDesign 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 by James Bennett, 17 years ago

On this and #6668, is there a good reason to avoid use of Python's own built-in "textwrap" module?

comment:3 by James Bennett, 17 years ago

Er, this and #6667 I mean.

comment:4 by Chris Beaven, 17 years ago

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 by Julien Phalip, 14 years ago

Type: Cleanup/optimization

comment:6 by Julien Phalip, 13 years ago

Severity: Normal

comment:7 by Alex Gaynor, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

Accepted, see #6667.

comment:8 by Aymeric Augustin, 12 years ago

Component: Core (Other)Utilities

comment:9 by Aymeric Augustin, 12 years ago

Patch needs improvement: set

The patch no longer applies, and the test must be rewritten as unittests.

comment:10 by Susan Tan, 11 years ago

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 by Susan Tan, 11 years ago

Owner: changed from Chris Beaven to Susan Tan

comment:12 by Susan Tan, 11 years ago

See PR: https://github.com/django/django/pull/1339 SmileyChris's patch works against existing utils_tests suite.

comment:13 by Tim Graham, 11 years ago

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 by Susan Tan, 11 years ago

Owner: Susan Tan removed
Status: assignednew

comment:15 by Markus Amalthea Magnuson, 10 years ago

Owner: set to Markus Amalthea Magnuson
Status: newassigned

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 by Markus Amalthea Magnuson, 10 years ago

Patch needs improvement: unset

comment:17 by Markus Amalthea Magnuson, 10 years ago

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 by Claude Paroz <claude@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In acb20016c0567876e7fc8144ecbe77905ee8388a:

Fixed #6668 -- Optimized utils.text wrap function

This fixes a failing test after applying an optimization of the
utils.text.wrap function by user SmileyChris.

Note: See TracTickets for help on using tickets.
Back to Top