[htmltmpl] Re: Patches for HTML-Template-2.5.tar.gz

[Sam T. <sa...@tr...>]

On Wed, 24 Jul 2002, Yunliang Yu wrote:

> Thanks for the wonderful HTML::Template module. I made a few changes
> while using it for my projects, hoping these changes will be useful for
> others:

Thanks!  I hope you don't mind, but I have some mixed feedback for you.
Please don't take it the wrong way - your work looks to be of excellent
quality.

> 1. add extended expression support without impact on speed

How did you test the speed of your changes?  Did you also check the memory
impact?  Changing VAR objects from scalars to hashes will probably greatly
expand the memory usage of large templates.

Also, you're using string eval() to execute expressions.  Are you really
sure that's faster than HTML::Template::Expr?  I wouldn't expect it to be,
but I could be wrong.  Also, do your changes pass the HTML::Template::Expr
test suite?

> 		<TMPL_IF NAME="item ne 'pizza' && price != 0.99">

I really don't like putting expressions into the NAME attribute.  I think
EXPR, as used in HTML::Template::Expr, is a better choice.

> 2. clean up global_vars implementation

Why did you force global_vars on for associate?  What does
explicit_global_vars do exactly?  It's not documented.

I think I'll need to read your patch a couple more times before I
understand exactly how your changes work.  Maybe you could give me a
summary?

> 3. add a new way to access outside variables without global_vars
>
> 		This is a normal variable: <TMPL_VAR NORMAL>.<P>
> 		<TMPL_LOOP NAME=FROOT_LOOP>
> 		   Use it inside the loop: <TMPL_VAR __NORMAL><P>
> 		</TMPL_LOOP>

I really don't like this.  How is an HTML designer supposed to understand
this bizarre syntax?

> 4. TMPL_VAR can now have a default value
>
>         <TMPL_VAR NAME="PARAMETER_NAME" VALUE="default value">

Some people will like this, I imagine.  Maybe call it "DEFAULT"?  I think
I might take this as a separate patch.  What do others think?

> 5. add TMPL_DEF tag for defining new parameters
>
>         <TMPL_DEF NAME="NAME" VALUE="value">

I don't like this.  Mixing display logic with setup of variables is a very
dangerous thing.

> 6. add __count__ to loops
>
>          <TMPL_VAR NAME="__count__">-th record in the loop.

Well, this something people are perpetually asking for, so I suppose I
should add it at some point.  It's just so easy to do when you need it
that I never considered it a priority.  Can you send me this one as a
separate patch too?

Thanks,
-sam