Opened 10 years ago

Closed 6 years ago

#3508 closed (fixed)

MergeDict needs more descriptive return values from __str__ and __repr__

Reported by: thebanana Owned by:
Component: Core (Other) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Since MergeDict is a subclass of object, it inherits the default str and repr methods. The default methods are very generic and not as useful as they could be, especially when you're trying to inspect objects.

I'm submitting a patch that gives a detailed representation of the instance data when you run str(myMergeDict) and returns an "eval-able" value when you run repr(myMergeDict).

Attachments (2)

doc_MergeDict.diff (1.0 KB) - added by thebanana 10 years ago.
patch to add str and repr to MergeDict
doc_MergeDict_py23.diff (1008 bytes) - added by thebanana 10 years ago.
MergeDict patch that works with Python 2.3

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by thebanana

Attachment: doc_MergeDict.diff added

patch to add str and repr to MergeDict

comment:1 Changed 10 years ago by (removed)

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Offhand, django is still targeting python 2.3; you're using a genexp in the repr which is py2.4. Bit of a no go.

Also, use iteritems. No point in having the interp. build the intermediate list...

comment:2 Changed 10 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Brian's comment is correct: the patch needs to work with Python 2.3.

comment:3 Changed 10 years ago by thebanana

OK, I am attaching another patch that I tested in Python 2.3. It uses a list expression in place of the genexp in the repr.

However, I'm not sure where I could use iteritems. MergeDict isn't like a normal dict. The data isn't stored the same, and MergeDict itself doesn't have an iteritems method. Could you be more specific about where you would use iteritems?

Changed 10 years ago by thebanana

Attachment: doc_MergeDict_py23.diff added

MergeDict patch that works with Python 2.3

comment:4 Changed 10 years ago by thebanana

An example of how this patch might be used:

> python2.3
Python 2.3.6 (#1, Feb 15 2007, 22:57:09)
[GCC 4.1.2 20060928 (prerelease) (Ubuntu 4.1.1-13ubuntu5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from django.utils.datastructures import MergeDict
>>> a = MergeDict({'a': 1, 'b': 2}, {'id': 'asdf', 'user': 'joe', 'color': 'blue'})
>>> str(a)
"{'a': 1, 'color': 'blue', 'b': 2, 'id': 'asdf', 'user': 'joe'}"
>>> repr(a)
"MergeDict({'a': 1, 'b': 2}, {'color': 'blue', 'id': 'asdf', 'user': 'joe'})"
>>>

comment:5 Changed 10 years ago by (removed)

No iteritems on MergeDict? Bah. Simple solution, add one ;)
Bit curious why MergeDict doesn't support the full dict protocol though; iteritems specifically would be useful compared to the current implementation.

Would use a map personally, but list comp seems preferred in the src.

Meanwhile, back to the patch- str output won't be valid;

>>> print MergeDict({1:2}, {1:3})
{1:3}
>>> print MergeDict({1:2}, {1:3})[1]
2

Fault lies with MergeDict diverting from the dict protocol; its items is a list built via L->R walk of the internal dicts, concating the list on the end as it goes. Dict instantiation works via just walking the args, adding them- meaning the rightmost key wins.

you want-

def __str__(self):
  l = self.items()
  l.reverse()
  return str(dict(l))

Although personally, I'd look at making MergeDict into a proper dict, and just using return str(dict(self.iteritems())) # till someone gets bored and implements it efficiently.

comment:6 Changed 10 years ago by thebanana

Brian,

I would also have implemented MergeDict differently. However, my intent for this patch was not to change how MergeDict behaved - just to add the "documenter" methods to make debugging and interactive inspection of request.REQUEST easier.

Grepping the code yielded the following example as the only way that MergeDict is used:

self._request = datastructures.MergeDict(self.POST, self.GET)

I'm happy to re-work the request.REQUEST and MergeDict implementations if that is what the community desires. Again, my initial goals were to minimize API impact while enhancing the inspection-related properties of the class.

comment:7 Changed 10 years ago by thebanana <john@…>

Also see the REQUEST docs: http://www.djangoproject.com/documentation/request_response/

They say the REQUEST searches POST first, then GET.

All-in-all, very low impact. I don't care if this gets merged in - I've even stopped using REQUEST myself. I just thought it might be useful to others. <shrug>

comment:8 Changed 10 years ago by anonymous

Needs tests: set
Patch needs improvement: unset

comment:9 Changed 9 years ago by Matt Boersma

Owner: changed from nobody to Matt Boersma
Status: newassigned

comment:10 Changed 9 years ago by Matt Boersma

Owner: Matt Boersma deleted
Status: assignednew

comment:11 Changed 6 years ago by Adam Nelson

Triage Stage: AcceptedReady for checkin

Aside from Brian Harring's comment, this patch is low impact and just does what it says, adds better str and repr and nothing else. The 2.4 version is sufficient now that 2.3 is no longer supported.

comment:12 Changed 6 years ago by Mike Lissner

Can we get this checked in? contrib.auth.views has several uses of request.REQUEST, and I just had the occasion to need to inspect them. Having this patch would have made life easier, for sure.

comment:13 Changed 6 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(In [13721]) Added more readable str and repr methods to MergeDict.
Thanks, john@…. Fixed #3508.

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