Fixed #6668 Optimise utils.text wrap #1339

Closed
wants to merge 12 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

ArcTanSusan commented Jul 7, 2013

https://code.djangoproject.com/ticket/6668 I tested SmileyChris's 5-yr old patch. The existing tests in utils_tests pass when this patch is implemented. Feel free to code review. Thanks.

timgraham and others added some commits Jul 4, 2013

Fixed #20561 -- Emphasized that QuerySet.distinct([*fields]) is only …
…supported by Postgres.

Thanks jtiai for the suggestion.
Fixed #20224 -- Update docs examples which mention __unicode__
Thanks Marc Tamlyn and Tim Graham for the review.
Deprecate usage of boolean value for widget attributes
Django 1.7 will loudly warn when widget attributes are assigned
boolean values. In Django 1.8, False will mean attribute is not
present while True will mean attribute present without value.
Refs #20684.
Fixed #14006 -- Documented that Field's 'description' attribute is in…
…terpolated with field.__dict__

Thanks abeld for the suggestion.
django/utils/text.py
- if len(lines) > 1:
- pos = len(lines[-1])
- yield word
+ for line in text.splitlines(True): # True keeps trailing linebreaks
@loic

loic Jul 9, 2013

Member

There are 3 spaces instead of 2 before inline comment.

django/utils/text.py
- pos = len(lines[-1])
- yield word
+ for line in text.splitlines(True): # True keeps trailing linebreaks
+ quote = ''
@loic

loic Jul 9, 2013

Member

quote isn't used anywhere.

django/utils/text.py
+ yield line
+ line = ''
+ break
+ yield '%s\n' % line[:space-1]
@loic

loic Jul 9, 2013

Member

Need spaces around - sign.

django/utils/text.py
+ quote = ''
+ max_width = (line.endswith('\n') and width + 1 or width)
+ while len(line) > max_width:
+ space = line[:max_width+1].rfind(' ') + 1
@loic

loic Jul 9, 2013

Member

Need spaces around + sign.

Member

loic commented Jul 9, 2013

I get a failing test: FAIL: test_wordcount (defaultfilters.tests.DefaultFiltersTests).

I didn't dig too deep, I'm not sure if we should pursue 100% compatibility in wrapping or just edit the test.

Contributor

ArcTanSusan commented Jul 9, 2013

Thanks for the code review, Loic. I implemented your formatting suggestions.

Member

loic commented Jul 9, 2013

You are welcome. Could you investigate if the text wrapping still makes sense after the patch? If the failing test is just due to a slight difference in the wrapping algorithm you may get away with updating the test; in that case you might want to check on the ticket if changing the test is deemed acceptable by most people.

Contributor

ArcTanSusan commented Jul 9, 2013

I get the same test fail in test_wordcount. I'll look into this patch more.

Owner

timgraham commented Aug 29, 2013

It looks like this introduces an off by one error as the failing test indicates. Please open a new PR if you can fix the patch, thanks!

@timgraham timgraham closed this Aug 29, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment