Opened 10 years ago

Closed 3 years ago

#6668 closed Cleanup/optimization (fixed)

Optimise utils.text wrap

Reported by: Chris Beaven Owned by: Markus Amalthea Magnuson
Component: Utilities Version: master
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 10 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 10 years ago.
and here's the patch

Download all attachments as: .zip

Change History (20)

Changed 10 years ago by Chris Beaven

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)

Changed 10 years ago by Chris Beaven

Attachment: faster_wrap.diff added

and here's the patch

comment:1 Changed 10 years ago by Chris Beaven

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 Changed 10 years ago by James Bennett

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

comment:3 Changed 10 years ago by James Bennett

Er, this and #6667 I mean.

comment:4 Changed 10 years ago by Chris Beaven

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

Type: Cleanup/optimization

comment:6 Changed 6 years ago by Julien Phalip

Severity: Normal

comment:7 Changed 6 years ago by Alex Gaynor

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

Accepted, see #6667.

comment:8 Changed 5 years ago by Aymeric Augustin

Component: Core (Other)Utilities

comment:9 Changed 5 years ago by Aymeric Augustin

Patch needs improvement: set

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

comment:10 Changed 4 years ago by Susan Tan

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

Owner: changed from Chris Beaven to Susan Tan

comment:12 Changed 4 years ago by Susan Tan

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

comment:13 Changed 4 years ago by Tim Graham

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

Owner: Susan Tan deleted
Status: assignednew

comment:15 Changed 3 years ago by Markus Amalthea Magnuson

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 Changed 3 years ago by Markus Amalthea Magnuson

Patch needs improvement: unset

comment:17 Changed 3 years ago by Markus Amalthea Magnuson

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

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