Code

Opened 15 months ago

Last modified 10 months ago

#20287 assigned New feature

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

Reported by: Keryn Knight <django@…> Owned by: cannona
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

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 cannona 14 months ago.
First stab at a patch.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 15 months ago by bmispelon

  • Cc bmispelon@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to New 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 Changed 15 months 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 15 months ago by bmispelon

  • Triage Stage changed from Unreviewed to Accepted

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

comment:4 Changed 15 months ago by bmispelon

For reference, #18105 seems related.

comment:5 Changed 14 months ago by cannona

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 Changed 14 months ago by cannona

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

comment:7 Changed 14 months ago by bmispelon

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 14 months ago by bmispelon (previous) (diff)

Changed 14 months ago by cannona

First stab at a patch.

comment:8 Changed 14 months ago by cannona

  • Has patch set
  • Needs tests set

Any feedback on this patch is welcome.

comment:9 Changed 10 months ago by FunkyBob

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from cannona to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.