Opened 7 years ago

Last modified 4 years ago

#20287 new New feature

BaseContext (and it's subclasses) lack emulation of dictionary items()

Reported by: Keryn Knight <django@…> Owned by:
Component: Template system Version: master
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no


Given the Context usually behaves like a dictionary (though underlying it is obviously also a stack), I was somewhat surprised that it has no items() nor iteritems() methods that would allow it to be iterated as one might expect to, though it does have an __iter__() method, which could probably be used if this has any merit.

Attachments (1)

20287.patch (2.5 KB) - added by Aaron Cannon 7 years ago.
First stab at a patch.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Type: UncategorizedNew feature

Since BaseContext is basically a ChainMap [1], I'd be +1 on the idea, but do you have an example showing how this feature might be useful?


comment:2 Changed 7 years ago by Keryn Knight <django@…>

I actually sort of do! Though it is arguably a quirk that has needed fixing for years at the debug_toolbar end too, and I'm tempted to file a bug there additionally, having finally sat down and figured (some of?) the problem out, accidentally, having been inspired to investigate when I was playing around with some code in a pdb session and tried to iterate over my context, and it threw an exception because the method I guessed would be there, wasn't.

In the template panel for debug_toolbar, it explicitly checks each context_layer (that is, each dict inside of the Context.dicts) if it hasattr('items') before iterating it, as a seemingly innocuous guard. One way in which the problem arises is once you are carrying around a Context, where one of the layers/dicts itself is a Context ... this admittedly is probably not an intended feature, but it works anyway because the inner context also emulates dictionary key/value pairs. Debug toolbar can no longer iterate and print that part of the template context, because it doesn't really know about bits of it (treats it as {}). This is especially apparent in things like django-CMS, where plugins will often render entirely without a context according to the debug_toolbar panel, because of the way contexts are used.

Basically though, any code which depends on, or makes assumptions about each Context dict being an actual-dict may fall over in such cases, but this is the only real world case for supporting more dictionary-like usage, in my experience.

comment:3 Changed 7 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted

Marking this as Accepted. It seems like a reasonable feature to add.

comment:4 Changed 7 years ago by Baptiste Mispelon

For reference, #18105 seems related.

comment:5 Changed 7 years ago by Aaron Cannon

I'm a bit confused on what iterkeys and the like should output. My first guess would be that it should iterate through the union of all the keys of the dictionaries in the stack of dicts, (I.E. no duplicate keys). itervalues should return the same value as calling getitem would for that particular key. However, this does not seem to jive with what itter is yielding, which is each dictionary in the stack of dicts.

Does it make sense to have iter return one thing, while itervalues and iterkeys returns something entirely different? My suggestion would be to add all the various basic map methods to iterate over the keys and values as I described at the beginning of this comment, , and changing iter to match, I.E. it would yield each key and value pair from the stack, where items in later dictionaries in the stack override the earlier ones, and all keys would be returned only once.

I'm just not sure if this would be likely to screw up existing code.

Implementing iterkeys and itervalues and other methods in RenderContext will be quite simple, as they can just proxy to self.dicts[-1].methodname. In fact, the get method that's already in there should probably do that as well, rather than rewriting logic already provided.

Please let me know if any of the above is not clear.


comment:6 Changed 7 years ago by Aaron Cannon

Owner: changed from nobody to Aaron Cannon
Status: newassigned

comment:7 Changed 7 years ago by Baptiste Mispelon

Like I mentionned in my first comment, I see BaseContext as an early implementation of a ChainMap.

Since ChainMap is python3-only, we can't really use it (yet) directly but I think it'd be good to copy its implementation so that switching to it will be easier when the time comes.

Also note that #20404 is closely related (implementing .keys() on context objects.

Forget what I said about #20404, it's not talking about the same thing at all.

Last edited 7 years ago by Baptiste Mispelon (previous) (diff)

Changed 7 years ago by Aaron Cannon

Attachment: 20287.patch added

First stab at a patch.

comment:8 Changed 7 years ago by Aaron Cannon

Has patch: set
Needs tests: set

Any feedback on this patch is welcome.

comment:9 Changed 7 years ago by FunkyBob

Just noting that currently ChainMap is implemented in python in py3, so it is back-portable to py2 almost verbatim.

comment:10 Changed 4 years ago by Tim Graham

Owner: Aaron Cannon deleted
Status: assignednew

We're dropping Python 2 in master in a couple weeks, so a patch that makes BaseContext subclass ChainMap (if that's appropriate, I haven't looked into the details) can be accepted then.

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