Opened 11 years ago

Closed 3 years ago

#20287 closed New feature (wontfix)

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

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

Description

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 11 years ago.
First stab at a patch.

Download all attachments as: .zip

Change History (12)

comment:1 by Baptiste Mispelon, 11 years ago

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?

[1] http://docs.python.org/3/library/collections.html#chainmap-objects

comment:2 by Keryn Knight <django@…>, 11 years ago

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 by Baptiste Mispelon, 11 years ago

Triage Stage: UnreviewedAccepted

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

comment:4 by Baptiste Mispelon, 11 years ago

For reference, #18105 seems related.

comment:5 by Aaron Cannon, 11 years ago

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.

Aaron

comment:6 by Aaron Cannon, 11 years ago

Owner: changed from nobody to Aaron Cannon
Status: newassigned

comment:7 by Baptiste Mispelon, 11 years ago

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.

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

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

by Aaron Cannon, 11 years ago

Attachment: 20287.patch added

First stab at a patch.

comment:8 by Aaron Cannon, 11 years ago

Has patch: set
Needs tests: set

Any feedback on this patch is welcome.

comment:9 by FunkyBob, 10 years ago

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

comment:10 by Tim Graham, 7 years ago

Owner: Aaron Cannon removed
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.

comment:11 by Mariusz Felisiak, 3 years ago

Has patch: unset
Needs tests: unset
Resolution: wontfix
Status: newclosed
Triage Stage: AcceptedUnreviewed

Closing as "wontfix" per David's comment.

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