Opened 18 years ago
Closed 18 years ago
#113 closed defect (fixed)
Problems with dict item access/assignment using attribute syntax
| Reported by: | darkness-keyword-genshi.e8bcba@… | Owned by: | cmlenz |
|---|---|---|---|
| Priority: | major | Milestone: | 0.4.1 |
| Component: | Expression evaluation | Version: | devel |
| Keywords: | genshi eval attr item dict assignment | Cc: |
Description
There are a few problems in genshi.template.eval related to the transformations that allow d.k to work like d["k"]. Here's a short-ish executable example:
from genshi.template import MarkupTemplate def run(content): return MarkupTemplate(content).generate().render() run(""" <html><head><title>Tests</title></head><body> <?python d = dict(k=1) # The most serious problem, IMHO, is that this doesn't work # because a Subscript node with the OP_ASSIGN flag isn't handled. d["k"] = 2 ?> <p> ${d.k} is still 1 (should be 2) </p> <?python # This assignment will produce an error because the AssAttr AST # node isn't handled. d.k = 3 ?> <p> ${d.k} should now be 3 </p> </body></html> """)
Executing this with Genshi 0.4 or SVN (as of r569) will cause an exception because of the d.k = 3; comment that out and you'll see the wrong values output in the template. There are other things that fail with the current Genshi too, mostly related to AST nodes that weren't included in genshi.template.eval; see the tests included in the patch for more examples.
The attached patch tries to solve these problems by essentially converting d.k to AccessWrapper(d).k and d["k"] to AccessWrapper(d)["k"]. AccessWrapper then tries both attribute and item access, in an order depending on which was used.
To me this patch looks a little ugly. (In my defense I think the original use of _lookup_attr/_lookup_item wasn't very pretty either. :) ) I assumed that converting all those _lookup_* calls to an object instantiation followed by another function call for the operator overload would consume more time and memory. However, in my testing with both Genshi from SVN versus my patched Genshi, I was unable to see a significant difference in either time taken or memory used. My speculation is that other modules used from genshi.template.eval are so much more costly (i.e. the compiler package) that any performance degradation due to my patch becomes immaterial.
Attachments (3)
Change History (8)
Changed 18 years ago by darkness-keyword-genshi.e8bcba@…
comment:1 Changed 18 years ago by Alec Thomas <alec@…>
I personally wouldn't expect the d.k = 3 to work if d was a dictionary, as my assumption is that code inside a <?python ... ?> PI is normal Python code, not Python-with-shortcuts.
I'd of course expect the d["k"] = 2 to work.
comment:2 Changed 18 years ago by Dale Sedivec <dale-keyword-genshi.e8bcba@…>
Actually, I didn't expect d.k = 3 to work in a <?python ?> section either. I didn't think to try it until I read the code and realized that both expressions in py:whatever tags as well as the code in those blocks were getting run through the same code transformation. Perhaps I wrongly lept to the conclusion that applying the transformations to code blocks was intentional, which is why I tried to fix that behavior with this patch rather than removing it altogether.
Personally I'd find it entirely acceptable if code blocks didn't get any special treatment at all: no interchangeable d.k/d["k"] kind of accesses, and no conversion of unknown names into Undefined objects (that is, no use of _lookup_name). If you stop genshi.template.eval from transforming code blocks, d["k"] = 2 works fine. Having d.k work in expressions is still nice, and I believe the current code works fine for that since you can't have things like assignments inside expressions.
comment:3 Changed 18 years ago by cmlenz
- Component changed from General to Expression evaluation
- Milestone set to 0.4.1
- Status changed from new to assigned
I agree that code blocks probably shouldn't be doing the item/attribute interchangeability magic, especially considering the complexity it adds to fix the various issues noted here.
I'm attaching an alternative proposal for a fix: ticket113.diff
Note that this still uses _lookup_name, whether it returns an Undefined object or raises an UndefinedError depends on whether you use strict or lenient lookup.
comment:4 Changed 18 years ago by cmlenz
See also #114.
comment:5 Changed 18 years ago by cmlenz
- Resolution set to fixed
- Status changed from assigned to closed
Patch to fix code transformation including item/attr assignment