velocity-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Nathan Bubna" <nbu...@gmail.com>
Subject Re: svn commit: r593549 - in /velocity/tools/branches/2.x/src/main/java/org/apache/velocity/tools: generic/ValueParser.java view/ParameterTool.java
Date Tue, 13 Nov 2007 00:18:31 GMT
On Nov 12, 2007 3:13 PM, Claude Brisson <claude@renegat.net> wrote:
> Le lundi 12 novembre 2007 à 10:53 -0800, Nathan Bubna a écrit :
> > it's not the name so much as how it's used.  it looks to me like once
> > it searches for subkey "foo", it won't even try to find subkey "bar"
> > or "woogie" or whatever.  either that's wrong or i'm missing
> > something?
>
> The "hasSubkeys" boolean is here so that we don't search for subkeys
> twice if we already know there isn't any (so that the overhead when
> looking for inexisting keys, in templates that don't use subkeys, is
> reduced to only one indeOf('.') in all key names). The very first
> implementation was buggy, it may be the one you read.

ah.  ok, looked closer at the latest version.  looks good. thx. :)
but now i have a new question...  why the expandSingletons stuff?  we
don't expand them for $params.foo, why should we expand them for
$params.foo.bar?

i'm assuming it's because request params automatically come as arrays
when you iterate over the parameter map.   but i would like
ValueParser to be more than just the base for ParameterTool, and it
doesn't make sense for ValueParser.

> > > > i'm also curious about what the advantages are of this approach (as
> > > > opposed to a ValueParserSub).  it's been a while since i've thought
> > > > through the implementation options for this.
> > >
> > > Things have changed since then: the engine is now quite efficient in
> > > automatic types conversions so the "foo.int" syntax is much less
> > > necessary than "foo.bar", and you always have "foo.getInt()".
> >
> > huh?  foo.int is the same as foo.getInt().  you can't have the latter
> > if the former doesn't work.  did you mean getInt('foo')?
>
> Yes, sorry.

no prob.  just want to make sure i understand.

> >   also, the
> > sub could handle both foo.bar and foo.int, and the engine can only do
> > a small fraction of the conversions the ValueParser is capable of.
> > so, while i don't see anything wrong with the current approach (apart
> > from the hasSubKey thing), i still suspect a Sub has more potential,
> > since we have complete control of the API.
> >
> > > But above all, having the generic getter return something else (that is,
> > > the ValueParserSub) than the underlying basic type leads - at least in
> > > my experience - to several side effects (like a jdbc driver being unable
> > > to handle the ValueParserSub class by calling toString on it).
> >
> > not sure how returning ValueParserSub is that different from returning
> > ValueParser.
>
> We only return a ValueParser for get("foo") when there are "foo.bar"
> keys, and keep returning the integral type otherwise. Since we expect
> the same fonctionnalities for $params and $params.foo, the recursive
> approach looked simpler to me.

perhaps from the standpoint of developing the tool itself, but neither
$params.getInt('foo.bar') nor $params.foo.getInt('bar') is as simple
to a user as $params.foo.bar.int

> Btw, I hoped that by having ValueParser implement Map I'd see
> ValueParser objects displayed like "{ key=value ... }" but I still see
> the ugly org.apache.velocity.tools.generic.ValueParser@848ecc, I'll dig
> into this...

nothing special about implementing the Map interface when it comes to
rendering.  Velocity simply renders stuff by calling toString().
That's all that needs to change to change the display.

> > > The proposed implementation only returns a new ValueParser object
> > > whenever subkeys are allowed and found.
> >
> > same could be done with a Sub.  granted, the implementation would have
> > to be smarter (i.e. search for a potential matching subkeys first)
> > than the last implementation for this, but there's no reason it can't
> > be done.
>
> Sure, but it all boils down to the decision to keep the foo.int syntax
> or not. If yes, then we definitely need a ValueParserSub object, if no,
> then returning either a new ValueParser (when subkeys are found) or the
> value in its integral type looks more natural to me.

yeah.  seems like just a question of whether we want the foo.int
syntax or not.  since it doesn't seem to be your itch, don't worry
about it.  if i get around to it, i'll do it.  if not, no big deal.

> My feeling here is that although foo.int is a cool syntax, it has too
> many backwards ; especially, we should always return the integral type
> (string, boolean or number) when available rather than a wrapper around
> it to avoid nasty side effects.

c'mon, ValueParser is no less a wrapper than ValueParserSub would be.
even a returning a simple HashMap would be returning a wrapper.   as
long as we make the subkey business configurable (with it off when in
deprecation support mode), and only return the
ValueParser/ValueParserSub/HashMap for $params.foo when there are keys
that start with "foo.", then i think we'll be fine.

<snip/>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org


Mime
View raw message