On 7/14/07, Brian Harring <[EMAIL PROTECTED]> wrote:
> 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).
If you follow the conversation to this point, you'll see that I
already raised this issue and plan to correct it; it's going to take
some alterations to the relevant code in django.template.__init__, but
ultimately the goal is to not have any unnecessary type
scanning/conversion happening.
> > 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)...
... and I really don't like the idea of passing around LiteralValues
in place of actual basic integers, strings, etc. If there's a way we
can avoid it, we should avoid it.
> 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).
"First make it work. Then make it right. Then make it fast."
No, I *haven't* timed it. Why? Err, I'm still writing it. :-) I'm
not obsessed with speed over functionality. I'll certainly start to
refine the speed once the functionality is all in place just how it
should be; I'm not going to worry about optimization prematurely.
Regarding instantiation, a hit there isn't exactly cause for alarm; a
once-per-run hit in exchange for the automatic argument handling is an
exchange I'll gladly make. As for invocation, again, I'll worry once
we cross that bridge.
> 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).
I'm not attached to the way constants are pulled in; I'm certainly
open to changes where they make sense.
> 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).
Giving the tag author total control over the parsing progress strikes
me as something of a red herring; hell, it's what I'm trying to get
*away* from -- worrying about that awful "if bit[3] == 'foo'"
boilerplate. Tag authors will always be welcome to write a tag the
old way if they want to retain total control.
That said, it may ultimately make more organizational sense anyway;
I'll keep it in mind as I'm refactoring.
> 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.
TemplateTag handles subclassed nodes just fine (we use template.Node
subclasses all over the place at the LJW); as far as further
subclassing a given Node written inside of a TemplateTag, one can
always write the subclass outside of TemplateTag, and hang it as an
attribute.
> 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.
Instantiation handles conversion to new-style when a parent class
isn't defined. I didn't want tag authors to be stuck always having to
write inner Node classes as subclasses; I felt this should be handled
automatically.
> 2) TemplateTag.__new__ is uneeded. Drop it.
You might be right; it may be a leftover from an earlier form of the
code. I'll look into 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.
Okay, I'll change these where it makes sense.
> 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.
Good point; I'll add some checking for these cases.
> 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. :)
It's not passing it in for a reason -- positional and keyword arguments
are handled differently.
> 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
a) INVALID_LIST_RE might be problematic for another reason anyway
(that being it may unfairly catch valid strings); I'll take a look at
it tonight and see if I can't handle it more cleanly.
b) I've honestly never liked re.match; as someone who writes regular
expressions constantly, I find it confusing for exactly the opposite
reason -- I expect a start-anchored regex to *start* with "^".
c) I took my lead for string escaping from
django.utils.text.smart_split, which is what Token.split_contents
already uses.
d) Bah, I *was* using float until Malcolm suggested Decimal. :-)
Perhaps a Decimal subclass is in order that plays nicely with float?
::shrug:: I'm in agreement with Malcolm's reasoning that we don't
want to lose precision if we can help it.
e) There is *not* nested list support; the outer sweep will catch all
list items, so inner sweeps will never match a list.
> 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).
I'm fine with changing the name.
> 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 :)
The gain, if it hasn't been clear up to this point, is in
automatically handling template tag arguments. We've been pretty sick
to death of writing argument parsing boilerplate for each new template
tag at the LJW. Trust me -- when you have *lots* of template tags, and
you're writing new ones all the time, you need all the
programmer-time-saving you can get. ;-)
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Django developers" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/django-developers?hl=en
-~----------~----~----~----~------~----~------~--~---
