Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13828 closed (fixed)

Refactor `dictsort` and `dictsortreversed` to make them more DRY

Reported by: suzaku Owned by: suzaku
Component: Template system Version: 1.2
Severity: Keywords: defaultfilters
Cc: satorulogic@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In django.template.defaultfilters, dictsort and dictsortreversed are almost the same except for the invocation of reverse() on the sorted list in the later function.

Now I have defined another function _dictsort(value, arg, reverse=False), and make dictsort and dictsortreversed call it.

Attachments (3)

13828_dry_dictsort.diff (1.4 KB) - added by suzaku 5 years ago.
13828_dry_dictsort_2.diff (1.5 KB) - added by d0ugal 5 years ago.
13828_dry_dictsort_3.diff (3.4 KB) - added by lrekucki 5 years ago.
Simplified patch.

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by suzaku

comment:1 Changed 5 years ago by PaulM

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by anonymous

It appears the diff attached was created from the template directory. diff's should be created against the base django directory.

Changed 5 years ago by d0ugal

comment:3 Changed 5 years ago by d0ugal

I've attached a patch that is the same but with the diff generated from the root directory.

I can also confirm that the patch cleanly applies to trunk and tests pass.

Changed 5 years ago by lrekucki

Simplified patch.

comment:4 Changed 5 years ago by lrekucki

  • Summary changed from [patch] Refactor `dictsort` and `dictsortreversed` to make them more DRY to Refactor `dictsort` and `dictsortreversed` to make them more DRY

Both functions turn into one-liners in Python 2.4, so a common function isn't needed. The previous patch also had a subtle change in behavior: the original list was modified by use of filter. Current patch returns a new list instead. Also modified tests to explicitly check that the original list isn't modified.

PS. There already is a "has patch" flag. You don't need to write "patch" in the topic.

comment:5 Changed 5 years ago by Alex

  • Resolution set to fixed
  • Status changed from new to closed

(In [15316]) Fixed #13828 -- DRY'd up the dictsort(reversed) filters, will speed them up as well.

comment:6 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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