On Fri, Jun 08, 2007 at 11:05:01PM -0000, SmileyChris wrote: > > On Jun 9, 9:32 am, Brian Harring <[EMAIL PROTECTED]> wrote: > > Realize it's hit trunk already, > No worries, we can always open another ticket :)
Already watching enough tickets ;) > > but noticed an annoying potential > > gotcha- for single var in a for loop, any changes to the context stay > > put- for multiple vars, the context gets wiped. > Dang, I knew there must have been some logic in why I was handling the > context originally - this was the case I had thought of then promptly > forgotten about. > > You're right. This is a change in behaviour between a single and > multiple keywords and probably should be addressed. > This would mean changing the context.update behaviour like mentioned > in the other ticket in this thread and and making sure that any > keywords not used this loop get popped out. Related to that, context.(push|pop) really ought to return the newly (add|remov)ed scope- if you have direct access to the underlying scope object, you can safely bypass the context stack object and abuse dict methods directly (its update, clear, etc specifically). 'Safely' in this case means "without having to know the internals of Context". Two upshots to it, you avoid involving Context.__.*item__ unless you specifically need it, more importantly it's a mild api tweak that is backwards compatible and removes the need to change the Context.update behaviour you're proposing. For consistancy, if they were changed Context.update ought to return the newly added scope, but again, it's backwards compatible- if anyone was already relying on Context.(push|pop|update) returning None, well, they're special and need to be wedgied for relying on basically a void return :) > > So... consistancy would be wise; which should it be? Replying to myself since I've had some time to think it through, yet to see any code that relies on the parent preserving random mappings in the context- thinking wiping the context per iteration might be wise. Tags shouldn't really be leaving random cruft in the parent context, thus cleansing the owned scope per iteration avoids the potential for screwups. Also has the benefit of simplifying ForNode.render while making it scale better (context.pop is a popleft on a list- meaning deeper the context stack, performance is linearly worse). > > Also, use izip instead of zip dang it :P > Gosh, how many keyword arguments are you trying to use? ;) > Does it really matter to use izip if it's going to usually be less > than 3 iterations? izip vs zip in this particular case is a rough 20% gain in izips favor for < 10 args raw izip/zip speed; honestly it's a background gain, it's a good habit to have however for dict bits (plus it was a joke, the real improvement is suggested above). Had a nice funny rant typed up related to the general lack of code efficiency in template code (ticket 4523 is an example), but instead just going to point out that adding new good habits to your repertoire isn't a bad thing. Knowing stuff like above is actually a *good* thing- no different then knowing that if len(sequence): pass is daft- the only time that's sane is if sequence.__nonzero__ isn't properly defined (which means it should be properly defined since it goes to the trouble of defining sequence.__len__ already- hacking around the lack of __nonzero__ via len is fugly). Better habit that avoids the extra func calls and extra object is- if sequence: pass As said, good habits. Simpler code, more often then not, faster. Using izip instead of zip for dict updates/creation is pretty much always a win (even on a single item), thus a similar 'good habit'. So as I said, "use izip instead of zip dang it :P". Good habits such as that helps to avoid "death by a thousand cuts" syndrome in your code. Lecture aside, opinions on the proposed scope change for ForNode.render from above is desired. ~harring
pgpPUctxrZRNo.pgp
Description: PGP signature
