You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2008/02/15 14:23:48 UTC
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental: src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/ xdocs/
In 15/02/2008, ebourg@apache.org <eb...@apache.org> wrote:
> Author: ebourg
> Date: Fri Feb 15 03:31:07 2008
> New Revision: 628021
>
> URL: http://svn.apache.org/viewvc?rev=628021&view=rev
> Log:
> The calendar objects are now formatted with their own time zone
> Removed the Java 1.3 workaround in PropertyListConfiguration to parse and format the dates without a SimpleDateFormat
>
BTW, SimpleDateFormat is not synchronized, so having package-protected
instances (whether static or not) may cause problems if the class is
intended for multithreaded use.
Also, the serialVersionUID should probably be changed as a new
instance field has been added.
Perhaps consider using org.apache.commons.lang.time.FastDateFormat
from Commons Lang for at least the formatting part?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental:
src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/
xdocs/
Posted by Emmanuel Bourg <eb...@apache.org>.
James Carman a écrit :
> Perhaps you could refactor your class a bit to make it more
> unit-testable. I've found rare occasions in the past where I had to
> expose a member variable to test cases, but for the most part, I could
> work around it by redesigning my class a bit. Is that possible in
> this case?
Maybe, suggestions are welcome. I could simply remove the test requiring
the package private visibility since another test covers the formatting
(testCalendar() in TestPropertyListConfiguration).
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental: src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/ xdocs/
Posted by James Carman <ja...@carmanconsulting.com>.
Perhaps you could refactor your class a bit to make it more
unit-testable. I've found rare occasions in the past where I had to
expose a member variable to test cases, but for the most part, I could
work around it by redesigning my class a bit. Is that possible in
this case?
On 2/15/08, sebb <se...@gmail.com> wrote:
> On 15/02/2008, Emmanuel Bourg <eb...@apache.org> wrote:
> > sebb a écrit :
> >
> >
> > > The static DateFormat is not private, so the synchronization is not guaranteed.
> >
> >
> > I'll make it private. I added a comment mentioning that it must be
> > synchronized.
> >
> >
> >
> > > Likewise the instance DateFormat.
> > >
> > > Indeed that is worse, since the format is temporarily changed by the
> > > private method - even if the field is made private, the method(s) that
> > > use it are potentially thread-hostile.
> > >
> > > Seems to me that the DateFormats should both be private.
> >
> >
> > I left the instance DateFormat package private only to access it from
> > the test cases. I'll synchronize its use as well.
> >
>
> The instance field ought to have a comment to say why it is package
> protected, and to point out that any multi-threaded use must be
> synchronized on DATE_FORMAT. The testcase may need to be updated
> accordingly.
>
> Should probably add an @GuardedBy() annotation as well.
>
>
> >
> >
> > >> The package name has changed, is it still necessary to change the UID ?
> > >>
> > >
> > > As far as I can tell the package name was not changed in this update,
> > > but if the previous version of the class was never deployed then it's
> > > probably not necessary.
> >
> >
> > Indeed, it's a newly created experimental branch.
> >
> >
> >
> > >> FastDateFormat could be useful to simplify the code on the 1.x branch,
> > >> but for the 2.x code base I think SimpleDateFormat is good enough.
> > >
> > > I don't follow that reasoning.
> >
> >
> > On the 1.x branch we were parsing a timezone with a SimpleDateFormat,
> > since this was not supported by Java 1.3 Oliver implemented an
> > alternative date parser. FastDateFormat could probably replace this
> > implementation since it supports the timezone and works on Java 1.3.
> >
> > On the 2.x branch Java 5 is the minimum requirement, so the Java 1.3
> > compatibility of FastDateFormat is not a compelling reason to adopt it.
> >
>
> From the Javadoc:
>
> "FastDateFormat is a fast and thread-safe version of SimpleDateFormat."
>
> This is why I suggested using it, not because it is 1.3 compatible.
>
> >
> > Emmanuel Bourg
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental: src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/ xdocs/
Posted by sebb <se...@gmail.com>.
On 15/02/2008, Emmanuel Bourg <eb...@apache.org> wrote:
> sebb a écrit :
>
>
> > The static DateFormat is not private, so the synchronization is not guaranteed.
>
>
> I'll make it private. I added a comment mentioning that it must be
> synchronized.
>
>
>
> > Likewise the instance DateFormat.
> >
> > Indeed that is worse, since the format is temporarily changed by the
> > private method - even if the field is made private, the method(s) that
> > use it are potentially thread-hostile.
> >
> > Seems to me that the DateFormats should both be private.
>
>
> I left the instance DateFormat package private only to access it from
> the test cases. I'll synchronize its use as well.
>
The instance field ought to have a comment to say why it is package
protected, and to point out that any multi-threaded use must be
synchronized on DATE_FORMAT. The testcase may need to be updated
accordingly.
Should probably add an @GuardedBy() annotation as well.
>
>
> >> The package name has changed, is it still necessary to change the UID ?
> >>
> >
> > As far as I can tell the package name was not changed in this update,
> > but if the previous version of the class was never deployed then it's
> > probably not necessary.
>
>
> Indeed, it's a newly created experimental branch.
>
>
>
> >> FastDateFormat could be useful to simplify the code on the 1.x branch,
> >> but for the 2.x code base I think SimpleDateFormat is good enough.
> >
> > I don't follow that reasoning.
>
>
> On the 1.x branch we were parsing a timezone with a SimpleDateFormat,
> since this was not supported by Java 1.3 Oliver implemented an
> alternative date parser. FastDateFormat could probably replace this
> implementation since it supports the timezone and works on Java 1.3.
>
> On the 2.x branch Java 5 is the minimum requirement, so the Java 1.3
> compatibility of FastDateFormat is not a compelling reason to adopt it.
>
>From the Javadoc:
"FastDateFormat is a fast and thread-safe version of SimpleDateFormat."
This is why I suggested using it, not because it is 1.3 compatible.
>
> Emmanuel Bourg
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental:
src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/
xdocs/
Posted by Oliver Heger <ol...@oliver-heger.de>.
Emmanuel Bourg schrieb:
> sebb a écrit :
>
>> The static DateFormat is not private, so the synchronization is not
>> guaranteed.
>
> I'll make it private. I added a comment mentioning that it must be
> synchronized.
>
>
>> Likewise the instance DateFormat.
>>
>> Indeed that is worse, since the format is temporarily changed by the
>> private method - even if the field is made private, the method(s) that
>> use it are potentially thread-hostile.
>>
>> Seems to me that the DateFormats should both be private.
>
> I left the instance DateFormat package private only to access it from
> the test cases. I'll synchronize its use as well.
>
>
>>> The package name has changed, is it still necessary to change the UID ?
>>>
>>
>> As far as I can tell the package name was not changed in this update,
>> but if the previous version of the class was never deployed then it's
>> probably not necessary.
>
> Indeed, it's a newly created experimental branch.
>
>
>>> FastDateFormat could be useful to simplify the code on the 1.x branch,
>>> but for the 2.x code base I think SimpleDateFormat is good enough.
>>
>> I don't follow that reasoning.
>
> On the 1.x branch we were parsing a timezone with a SimpleDateFormat,
> since this was not supported by Java 1.3 Oliver implemented an
> alternative date parser. FastDateFormat could probably replace this
> implementation since it supports the timezone and works on Java 1.3.
>
> On the 2.x branch Java 5 is the minimum requirement, so the Java 1.3
> compatibility of FastDateFormat is not a compelling reason to adopt it.
>
> Emmanuel Bourg
>
According to the Javadocs, FastDateFormat is all about formatting, but
does not support parsing. That's why I wrote a parser myself.
Oliver
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental:
src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/
xdocs/
Posted by Emmanuel Bourg <eb...@apache.org>.
sebb a écrit :
> The static DateFormat is not private, so the synchronization is not guaranteed.
I'll make it private. I added a comment mentioning that it must be
synchronized.
> Likewise the instance DateFormat.
>
> Indeed that is worse, since the format is temporarily changed by the
> private method - even if the field is made private, the method(s) that
> use it are potentially thread-hostile.
>
> Seems to me that the DateFormats should both be private.
I left the instance DateFormat package private only to access it from
the test cases. I'll synchronize its use as well.
>> The package name has changed, is it still necessary to change the UID ?
>>
>
> As far as I can tell the package name was not changed in this update,
> but if the previous version of the class was never deployed then it's
> probably not necessary.
Indeed, it's a newly created experimental branch.
>> FastDateFormat could be useful to simplify the code on the 1.x branch,
>> but for the 2.x code base I think SimpleDateFormat is good enough.
>
> I don't follow that reasoning.
On the 1.x branch we were parsing a timezone with a SimpleDateFormat,
since this was not supported by Java 1.3 Oliver implemented an
alternative date parser. FastDateFormat could probably replace this
implementation since it supports the timezone and works on Java 1.3.
On the 2.x branch Java 5 is the minimum requirement, so the Java 1.3
compatibility of FastDateFormat is not a compelling reason to adopt it.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental: src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/ xdocs/
Posted by sebb <se...@gmail.com>.
On 15/02/2008, Emmanuel Bourg <eb...@apache.org> wrote:
> sebb a écrit :
>
>
> > BTW, SimpleDateFormat is not synchronized, so having package-protected
> > instances (whether static or not) may cause problems if the class is
> > intended for multithreaded use.
>
>
> I made so the usage of the static DateFormat is synchronized. The
> instance DateFormat isn't synchronized but its use by concurrent threads
> is unlikely. I can change it though if it's really an issue.
>
The static DateFormat is not private, so the synchronization is not guaranteed.
Likewise the instance DateFormat.
Indeed that is worse, since the format is temporarily changed by the
private method - even if the field is made private, the method(s) that
use it are potentially thread-hostile.
Seems to me that the DateFormats should both be private.
Also, unless the access is synchronized, any public methods that use
the instance format should be documented as not being thread safe (in
fact thread-hostile).
>
> > Also, the serialVersionUID should probably be changed as a new
> > instance field has been added.
>
>
> The package name has changed, is it still necessary to change the UID ?
>
As far as I can tell the package name was not changed in this update,
but if the previous version of the class was never deployed then it's
probably not necessary.
>
> > Perhaps consider using org.apache.commons.lang.time.FastDateFormat
> > from Commons Lang for at least the formatting part?
>
>
> FastDateFormat could be useful to simplify the code on the 1.x branch,
> but for the 2.x code base I think SimpleDateFormat is good enough.
I don't follow that reasoning.
> Emmanuel Bourg
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r628021 - in /commons/proper/configuration/branches/configuration2_experimental:
src/main/java/org/apache/commons/configuration2/plist/ src/test/java/org/apache/commons/configuration2/plist/
xdocs/
Posted by Emmanuel Bourg <eb...@apache.org>.
sebb a écrit :
> BTW, SimpleDateFormat is not synchronized, so having package-protected
> instances (whether static or not) may cause problems if the class is
> intended for multithreaded use.
I made so the usage of the static DateFormat is synchronized. The
instance DateFormat isn't synchronized but its use by concurrent threads
is unlikely. I can change it though if it's really an issue.
> Also, the serialVersionUID should probably be changed as a new
> instance field has been added.
The package name has changed, is it still necessary to change the UID ?
> Perhaps consider using org.apache.commons.lang.time.FastDateFormat
> from Commons Lang for at least the formatting part?
FastDateFormat could be useful to simplify the code on the 1.x branch,
but for the 2.x code base I think SimpleDateFormat is good enough.
Emmanuel Bourg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org