[Django] #20287: BaseContext (and it's subclasses) lack emulation of dictionary items()
Django
-----------------------------------------+--------------------
Reporter: Keryn Knight <django@…> | Owner: nobody
Type: Uncategorized | Status: new
Component: Template system | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* cc: bmispelon@… (added)
* needs_better_patch: => 0
* type: Uncategorized => New feature
* needs_tests: => 0
* needs_docs: => 0
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:1>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:2>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: New feature | Version: master
Component: Template system | Resolution:
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* stage: Unreviewed => Accepted
Comment:
Marking this as `Accepted`. It seems like a reasonable feature to add.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:3>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by bmispelon):
For reference, #18105 seems related.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:4>
Django
Reporter: Keryn Knight | Owner: nobody
<django@…> | Status: new
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:5>
Django
Reporter: Keryn Knight | Owner: cannona
<django@…> | Status: assigned
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => cannona
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:6>
Django
Reporter: Keryn Knight | Owner: cannona
<django@…> | Status: assigned
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:7>
Django
Reporter: Keryn Knight | Owner: cannona
<django@…> | Status: assigned
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by cannona):
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
Any feedback on this patch is welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:8>
Django
Reporter: Keryn Knight | Owner: cannona
<django@…> | Status: assigned
Type: New feature | Version: master
Component: Template system | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by FunkyBob):
Just noting that currently ChainMap is implemented in python in py3, so it
is back-portable to py2 almost verbatim.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:9>
Django
Reporter: Keryn Knight | Owner: (none)
<django@…> |
Type: New feature | Status: new
Component: Template system | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* owner: Aaron Cannon => (none)
* status: assigned => new
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:10>
Django
Reporter: Keryn Knight | Owner: (none)
<django@…> |
Component: Template system | Version: master
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* has_patch: 1 => 0
* resolution: => wontfix
* needs_tests: 1 => 0
* stage: Accepted => Unreviewed
Comment:
Closing as "wontfix" per
[https://github.com/django/django/pull/13953#issue-564197873 David's
comment].
--
Ticket URL: <https://code.djangoproject.com/ticket/20287#comment:11>