On Fri, Jul 13, 2007 at 11:19:53PM -0500, Tom Tobin wrote:
>
> On 7/12/07, Malcolm Tredinnick <[EMAIL PROTECTED]> wrote:
> >
> > A few comments (mostly questions) from an initial reading:
>
> 3) Regarding Brian Harring's #3453, the type conversion logic is
> redundant between his approach and mine. taghelpers already avoids
> the issue he's trying to bypass, as it performs type conversion in one
> initial sweep and stores the literal values for later. I'd rather not
> use his approach in taghelpers; I don't like the idea of passing
> around LiteralVariables when I can be passing around
> honest-to-${deity} integers, strings, etc. :-)
You actually *do* want to use my approach. Note the "resolve_variable
leads to redundant work" summary- the up front 'type conversion'
(literal/constant conversion is a bit more accurate) check is done
every time parsing occurs; that patch just converts parsing down
to a one time cost. The result of this is to speed up template
variable lookup.
For your approach, normal usage of a node with args render triggers
self.resolve_context, which in turn invokes module level
resolve_context, which in turn invokes resolve_variable, which in
turn does the conversion every render.
In other words, the purpose of my patch *still* exists- nuke the
redundant reparsing of the var path. Your patches implementation
means this gain isn't possible, since the re-parsing is forced
everytime (further, the basic algo of it is duplicated in at least 2
spots from what I can tell).
> Ideally, I'd like to yank the type conversion logic out of
> resolve_variable, as I believe type conversion and variable resolution
> are really two separate things and should be available separately as
> needed.
#3453s patch already *does* this; two classes, LiteralValue (this is
the value to shove in everytime this node is rendered), and
ChainedLookupVariable (do a context lookup)...
Meanwhile, back to your basic patch... while this is interesting
code, frankly I'm not seeing the gain in it. Few reasons;
1) Gurantee you've not timed this sucker. I suggest you do so, it's
going to be a noticable hit to tag instantiation, and more importantly
tag invocation- 2x increase on simple tags wouldn't surprise me.
Reasoning pretty much comes down to the overhead involved, and the
fact your base code tries to assign everything back to the node
instead of just returning back to the render func. If the node needs
to preserve state, let its' render do it- don't mutate the node every
run (it's slower, and opens up an angle for bugs).
2) Needless complexity. Example:
class SomeNode(TemplateTag):
class Node:pass
class Constants:
foo = 1
foo2 = 2
becomes
SomeNode.constants['foo']. *cough* why not the following...
class SomeNode(TemplateTag):
class Node:pass
constants = {'foo':1, 'foo2':2}
Latter is the same, using normal pythonic constructs people know/love,
and doesn't abuse classes as a forced indent (basically).
3) extension of #2; while Model does make *good* use of metaclass/sub
classes shoved into a class, it's also far more complex. This code
could pretty easily have all of this shoved into __init__ of a target
node, thus getting rid of the seperated parser func; instead, just
register the class itself (which would handle the parsing and
rendering)- fair bit simpler, via that approach, far more flexible
also (tag author has total control of __init__, thus can do whatever
they want).
4) Strongly doubt subclassing of TemplateTag nodes (as in, second
level) is possible/works correctly, going by the arg_list bits-
willing to bet a failed parsing of a node trashes that node classes
ability to make new instances of itself also due to mangling of the
class for each attempted instance generation.
Errors/issues I spotted follow; suspect there are more, but it's 2.5k
lines, lot to dig through (that and the instantiation logic is _not_
simple to follow):
1) 'class Node:pass' from above is actually incorrect;
'class Node(object):pass' is actually what's required if you're being
careful, since your code tries to mixin Node, which is newstyle- you
can get odd interactions mixing old style in with new, if one is new,
all should be new.
2) TemplateTag.__new__ is uneeded. Drop it.
3) use itervalues(), iteritems(). Seriously, there isn't any reason
to use the others if you're just iterating over the mapping without
mutating it.
4) 1729, identification of the 'validators'. The code will misbehave
with 'validate_spork_' and 'validate_spork'; dicts have no guranteed
ordering, meaning that which validator would be used there can vary-
thats ignoring the fact its allowing two validator methods to map to
the same arg.
5) line 1890 of your patch, 'while self.arg_list'. Pass the list into
process_arg instead of having arg_list silently screw with
self.arg_list on it's own; at least via passing it in, you know it may
mutate it. Not particularly sure how that code is working offhand
either. :)
6) line 1573; parse_arguments; regex isn't necessarily your friend
(comments inlined)-
def parse_arguments(arg_string):
# this search is redundant. Checking each arg, this can be spotted
# pretty easily as it parses.
if INVALID_LIST_RE.search(arg_string):
raise TemplateSyntaxError("Invalid list argument")
arg_matches = list(ARG_RE.finditer(arg_string))
output = []
for arg in arg_matches:
output.append(convert_argument(arg.group()))
return output
def convert_argument(arg_string):
# search does what it sounds like; find a match, not necessarily
# starting at 0. Use match instead- it at least gurantees anchoring,
# rather then requiring the reviewer to check each regex to see if
# it's properly anchored.
if STRING_RE.search(arg_string):
# \" -> " is an unstated change in behaviour, debatable if desired
# (just use the opposite quoting should cover most cases)
return arg_string[1:-1].replace('\\%s' % arg_string[0],
arg_string[0]).replace('\\\\', '\\')
if NUMBER_RE.search(arg_string):
# Decimal is an unstated change; further, it isn't friendly from a
# backwards compatibility standpoint since Decimal(1.0) + 1.0 ==
# exception
return '.' in arg_string and Decimal(arg_string) or arg_string)
if KEYWORD_RE.search(arg_string):
return TemplateTagKeyword(arg_string)
if LIST_RE.search(arg_string):
# this adds nested list support; TemplateTag.__doc__ indicates however
# nest listed support isn't supported... which is it?
listout = []
s = arg_string
while s:
hit = BASIC_RE.match(s)
if hit is None:
raise TemplateSyntaxError("Invalid list argument")
g = hit.group()
listout.append(convert_argument(g))
s = s[len(g):]
s = re.sub(r'^[\s,]+', '', s)
return listout
# treat as a string
return arg_string
7) TemplateTagKeyword (aka, unicode) is badly named; 'Keyword' has a
specific meaning in most python code, usage here doesn't really
reflect that (better name is something string related). General
Keyword usage in this patch suffers the same- at least to me, 'arg'
means positional, keyword means 'foon=blah' in invocation, aka
optional args- yes you can pass positional that way, but it's pretty
rare (and breaks down under '*arg' usage pretty quickly).
Providing examples of where your templatetags shine would be wise; at
this point, it looks like just shy of 600 lines of fairly tricky to
follow code doing class mangling for what at least to me, doesn't seem
like that much of a problem. Will do a more thorough dig through at
some point, but I'd like to see examples of the gain here prior :)
~harring
pgp2SZHeyWI3L.pgp
Description: PGP signature
