Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4506 closed (fixed)

regroup should rely on __eq__ instead of repr

Reported by: (removed) Owned by: adrian
Component: Template system Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

Tucked away in django.template.defaulttags.RegroupNode.render is this lil gem-

for obj in obj_list:
    grouper = self.expression.resolve(obj, True)
    # TODO: Is this a sensible way to determine equality?
    if output and repr(output[-1]['grouper']) == repr(grouper):
        output[-1]['list'].append(obj)
    else:
        output.append({'grouper': grouper, 'list': [obj]})

re: the TODO... the answer is 'no'.

Aside from the fact it's forcing 2(n-1) repr when n repr would suffice, equality protocol exists exactly for usage in cases like this. The repr abusage there covers up a few things-
1) bad object implementations that have taken the time to flesh out repr, but not eq which isn't really covering up. Tough cookies frankly (eq/ne are simple methods to implement, and are needed elsewhere- hashing for example).
2) Objects that repr to the same, but have an eq that says they differ. If eq says it differs, it differs- using an alt equality method is dodging whatever reason the original author had for having eq compare differently (exempting if they just didn't implement eq, which is scenario #1).

Either way, the extra repr calls aren't needed- more importantly, repr should *not* be used there.

So... counter args?

Attachments (2)

RegroupNode-groupby.patch (1.5 KB) - added by (removed) 8 years ago.
switch to doing eq comparisons, drop the duplicate grouping logic instead using itertools.groupby
RegroupNode-groupby-v2.patch (2.0 KB) - added by (removed) 8 years ago.
updated version that works for py2.3 (added a native groupby equivalent).

Download all attachments as: .zip

Change History (10)

comment:1 follow-up: Changed 8 years ago by mtredinnick

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

Nice catch. Glad that you're continuing to review this code. Thanks.

I think you're possibly worrying too much about micro-optimisations at this sort of level with the 2n vs n thing, since that isn't where the bulk of the time budget in a request goes, unfortunately, but it obviously can't hurt if it doesn't make the code any less readable or easily maintainable (so don't stop doing them for that reason).

The correctness argument looks 100% valid, though and is worth changing. Looks like 'grouper' can be any Python object, so we can't make assumptions about its behaviour.

comment:2 in reply to: ↑ 1 Changed 8 years ago by (removed)

Replying to mtredinnick:

Nice catch. Glad that you're continuing to review this code. Thanks.

I think you're possibly worrying too much about micro-optimisations at this sort of level with the 2n vs n thing, since that isn't where the bulk of the time budget in a request goes, unfortunately, but it obviously can't hurt if it doesn't make the code any less readable or easily maintainable (so don't stop doing them for that reason).

The 2n -> n reduction is a fallback suggestion should someone have a legitimate reason to preserve the repr usage; may not seem like much, but if profiling points to it... well, hard to argue against a measurable gain (helps to assume the gain comes from an odd usage scenario having around 20k regroups). It helps also that the speed up there come doesn't care if it's repr or eq also ;)

Either way, rest assured, I'll be bugging the hell out of y'all with large scale changes in the near future (unfortunately, those target memory usage, not runtime).

The correctness argument looks 100% valid, though and is worth changing. Looks like 'grouper' can be any Python object, so we can't make assumptions about its behaviour.

Only possible scenario I could see for where shifting away from repr might cause issues is for when the groupper object is a model instance- would be amazed if someone was relying on that quirk however.

Changed 8 years ago by (removed)

switch to doing eq comparisons, drop the duplicate grouping logic instead using itertools.groupby

comment:3 Changed 8 years ago by (removed)

  • Has patch set

comment:4 Changed 8 years ago by mtredinnick

  • Patch needs improvement set

Since itertools.groupby doesn't exist in Python 2.3, we'll also need to include a fallback for that case. The Python docs include an implementation.

It's getting to where we should just start up django.utils.fallback or something to contain all these pieces. I'll fix this up when I check this in unless somebody beats me to it.

Changed 8 years ago by (removed)

updated version that works for py2.3 (added a native groupby equivalent).

comment:5 Changed 8 years ago by mtredinnick

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

(In [5484]) Fixed #4506 -- Changed "regroup" template tag to use eq instead of repr()
for grouping equality checking. Thanks, Brian Harring.

comment:6 Changed 8 years ago by (removed)

  • Resolution fixed deleted
  • Status changed from closed to reopened

Relevant once again since [5484] was (rightfully) punted due to causing a bit 'o mayhem :)

comment:7 Changed 8 years ago by mtredinnick

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

We didn't revert [5484]; we reverted [5482], but only that changeset (not subsequent changes).

comment:8 Changed 8 years ago by mtredinnick

(In [5526]) Fixed small bug in Python 2.3 fallback for itertools.groupby. Refs #4506.

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