regroup should rely on __eq__ instead of repr

[(removed)]

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?

[Malcolm Tredinnick]

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.

[(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.

[(removed)]

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

[Malcolm Tredinnick]

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.

[(removed)]

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

[Malcolm Tredinnick]

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

[(removed)]

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

[Malcolm Tredinnick]

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

[Malcolm Tredinnick]

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