Opened 8 years ago

Closed 5 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 8 years ago.
patch to add str and repr to MergeDict
doc_MergeDict_py23.diff (1008 bytes) - added by thebanana 8 years ago.
MergeDict patch that works with Python 2.3

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by thebanana

patch to add str and repr to MergeDict

comment:1 Changed 8 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 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 8 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 8 years ago by thebanana

MergeDict patch that works with Python 2.3

comment:4 Changed 8 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 8 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 8 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 8 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 8 years ago by anonymous

  • Needs tests set
  • Patch needs improvement unset

comment:9 Changed 8 years ago by mboersma

  • Owner changed from nobody to mboersma
  • Status changed from new to assigned

comment:10 Changed 8 years ago by mboersma

  • Owner mboersma deleted
  • Status changed from assigned to new

comment:11 Changed 5 years ago by adamnelson

  • Triage Stage changed from Accepted to Ready 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 5 years ago by mlissner

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 5 years ago by mtredinnick

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

(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