Re: svn commit: r639770 - in /turbine/core/branches/TURBINE_2_3_BRANCH: src/java/org/apache/turbine/util/parser/ src/test/org/apache/turbine/util/parser/ xdocs/

[Scott Eade <se...@backstagetech.com.au>]

tv@apache.org wrote:
> Author: tv
> Date: Fri Mar 21 12:21:55 2008
> New Revision: 639770
>
> URL: http://svn.apache.org/viewvc?rev=639770&view=rev
> Log:
> Made ValueParser and BaseValueParser locale-aware. Note that the default locale used is always the default value of the JVM which is different from the previous behaviour where the locale used was sometimes Locale.US (for float, double and BigDecimal) and sometimes the JVM-default (for dates).
>
> TODO: Integration into RunData
>
> Modified:
>     turbine/core/branches/TURBINE_2_3_BRANCH/src/java/org/apache/turbine/util/parser/BaseValueParser.java
>     turbine/core/branches/TURBINE_2_3_BRANCH/src/java/org/apache/turbine/util/parser/ValueParser.java
>     turbine/core/branches/TURBINE_2_3_BRANCH/src/test/org/apache/turbine/util/parser/BaseValueParserTest.java
>     turbine/core/branches/TURBINE_2_3_BRANCH/xdocs/changes.xml
>   
<snip/>
I have had a look at this and have the following comments:

   1. I think NumberFormat.parse() will convert "12.3foo" to 12.3 where
      as the old Double.parseDouble() would throw a
      NumberFormatException. I don't think we should be blindly dropping
      anything after the number so I think we need to detect this and
      throw an exception.  This also applies to the corresponding Float
      and BigDecimal methods.
   2. Why doesn't getDoubleObject() just invoke getDouble()? 
      getDoubleObjects() use getDoubles()?  Being pedantic, getDouble()
      and getDoubles() have a common block of code that could be in a
      private method.  This would apply to many of the other methods
      also.  Whether or not these should be addressed is a matter of
      style - clearly they were like this before your changes.

Try adding the following to BaseValueParserTest.testGetDouble():
        parser.add("unparsable2", "1a");
        result = parser.getDouble("unparsable2");
        assertEquals(result, 0, 0);

The TurbineRunDataService change looks okay, but I will try it out 
shortly to confirm that I can easily override the locale on a per user 
basis.

Scott

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

Re: svn commit: r639770 - in /turbine/core/branches/TURBINE_2_3_BRANCH: src/java/org/apache/turbine/util/parser/ src/test/org/apache/turbine/util/parser/ xdocs/

[Scott Eade <se...@backstagetech.com.au>]

Thomas Vandahl wrote:
> Scott Eade wrote:
>> I have had a look at this and have the following comments:
> Thank you for your review.
>>   1. I think NumberFormat.parse() will convert "12.3foo" to 12.3 where
>>      as the old Double.parseDouble() would throw a
>>      NumberFormatException. I don't think we should be blindly dropping
>>      anything after the number so I think we need to detect this and
>>      throw an exception.  This also applies to the corresponding Float
>>      and BigDecimal methods.
> That would involve parsing with ParsePosition, right?
That looks right.  Basically we need to make sure that all of the String 
was used to produce the number.
>>   2. Why doesn't getDoubleObject() just invoke getDouble()?      
>> getDoubleObjects() use getDoubles()?  Being pedantic, getDouble()
>>      and getDoubles() have a common block of code that could be in a
>>      private method.  This would apply to many of the other methods
>>      also.  Whether or not these should be addressed is a matter of
>>      style - clearly they were like this before your changes.
> No, they weren't. I just checked. Actually I changed very little in 
> the logic of the methods. But yes, you are right, that'd be less code 
> and less code is a Good Thing (TM). As numberFormat.parse() returns a 
> Number, it would be even possible to share the code between Float, 
> float, Double, and double. I'll try to fix this over the weekend.
Crossed wires I think - my comment was that the code was a bit sloppy 
_before_ you touched it.  You made it no worse than it already was, but 
tidying it up will be a good thing.
>> Try adding the following to BaseValueParserTest.testGetDouble():
>>        parser.add("unparsable2", "1a");
>>        result = parser.getDouble("unparsable2");
>>        assertEquals(result, 0, 0);
> Will do.
It should fail until the ParsePosition changes that confirm the entire 
String us used are made.
>> The TurbineRunDataService change looks okay, but I will try it out 
>> shortly to confirm that I can easily override the locale on a per 
>> user basis.
> That's exactly my problem, too. As I see it, the Turbine request 
> processing is missing a call to data.setLocale() at some (early) 
> place. I'd propose to add a method to the LocalizationService, 
> something like setRunDataLocale(RunData) which can be overridden in 
> the implementation.
Here is what I currently do (I require per user locales):

    * My User implementation has a getLocale() method.
    * When my custom LocalizationTool.init() is executed it pulls the
      User from RunData, grabs the locale and uses this (my
      LocalizationTool also implements a formatPrefixed() method that
      can handle an arbitrary number of parameters embedded in a single
      string, space separated following the key).  So in my templates I
      can use $l10n and the locale for the user will be used.
    * In Java code I do a number of things:
          o return just l10n keys and defer the lookups to the templates
            (best answer, but certainly need something akin to my
            formatPrefixed method)
          o use the Localization methods that take a locale argument
            that I first retrieve from the User which is obtained from
            RunData (better to leave this to the presentation layer, but
            sometimes it is necessary)
          o in some places it appears I have insanely initialized and
            used LocalizationTool (bad!)

As I see it, RunData is currently initialized (and the locale set to 
ParameterParser and CookieParser) before my user specific locale is 
available.  RunData doesn't see the user until populate() is invoked by 
the SessionValidator.  So the best I can do is to override 
RunData.populate() to then set the locale of the PP and CP.  I would see 
RunData.setLocale() updating the PP and CP locales if these are not null 
(TurbineRunDataService.getRunData() is currently going the other way and 
using RunData as the source of the locale for PP and CP - as you say, 
these need to get a default from somewhere).

So in what order do these things happen?  I won't have the true locale 
set in RunData until SessionValidator invokes RunData.populate().  If 
LocalizationTool.init() is not invoked until after this I would no 
longer have to set the locale explicitly - it could just pull the value 
from RunData.  In Java code I can if necessary pull the locale from 
RunData when retrieving formatted strings via Localization.  Are PP and 
CP used in any significant way prior to RunData.populate() being 
invoked?  Not likely or rather not in order to retrieve anything that 
requires localization.

So given your suggestion for providing 
LocalizationService.setRunDataLocale(RunData), which would just provide 
a default in my case until populate() can set the user specific value, 
where would *that* be invoked?  Thinking in the realm of removing rather 
than adding dependencies, how about RunData just duplicate the mechanism 
that LocalizationService uses to get it's default locale - otherwise 
there will be a messy optional dependency that will still need to come 
up with a fall back default if the LocalizationService is unavailable.

This is kind of a bunch of fairly unstructured thoughts and 
observations.  My apologies for the level of randomness and for not 
getting to this earlier in the week.

Scott

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

Re: svn commit: r639770 - in /turbine/core/branches/TURBINE_2_3_BRANCH: src/java/org/apache/turbine/util/parser/ src/test/org/apache/turbine/util/parser/ xdocs/

[Thomas Vandahl <th...@tewisoft.de>]

Scott Eade wrote:

> I have had a look at this and have the following comments:

Thank you for your review.

>   1. I think NumberFormat.parse() will convert "12.3foo" to 12.3 where
>      as the old Double.parseDouble() would throw a
>      NumberFormatException. I don't think we should be blindly dropping
>      anything after the number so I think we need to detect this and
>      throw an exception.  This also applies to the corresponding Float
>      and BigDecimal methods.

That would involve parsing with ParsePosition, right?

>   2. Why doesn't getDoubleObject() just invoke getDouble()?      
> getDoubleObjects() use getDoubles()?  Being pedantic, getDouble()
>      and getDoubles() have a common block of code that could be in a
>      private method.  This would apply to many of the other methods
>      also.  Whether or not these should be addressed is a matter of
>      style - clearly they were like this before your changes.

No, they weren't. I just checked. Actually I changed very little in the 
logic of the methods. But yes, you are right, that'd be less code and 
less code is a Good Thing (TM). As numberFormat.parse() returns a 
Number, it would be even possible to share the code between Float, 
float, Double, and double. I'll try to fix this over the weekend.

> Try adding the following to BaseValueParserTest.testGetDouble():
>        parser.add("unparsable2", "1a");
>        result = parser.getDouble("unparsable2");
>        assertEquals(result, 0, 0);

Will do.

> The TurbineRunDataService change looks okay, but I will try it out 
> shortly to confirm that I can easily override the locale on a per user 
> basis.

That's exactly my problem, too. As I see it, the Turbine request 
processing is missing a call to data.setLocale() at some (early) place. 
I'd propose to add a method to the LocalizationService, something like 
setRunDataLocale(RunData) which can be overridden in the implementation.

Bye, Thomas.


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