Overall, I think the changes are good. Ceki will need to review the changes
in the various date format helper classes.
What I did was check out the latest code from the main branch, copy in the
changed files, and then reviewed them using the WinMerge tool to compare the
files side-by-side. I successfully compiled the code. There were some
issues with the test cases (see below).
PatternLayout.java
- I think the contributors list should be moved to the javadoc area so that
deserved recognition is awarded. This is not specific to this file and has
nothing to do with your changes. Just an observation.
- "<li>Any pattern accpetible", should be "acceptible".
- The private timezone member was removed. Was it not used?
-----
AbsoluteDateFormat.java
- If I use the setDecimalSeparatorForLocale() method, what should be the
value of the decSepSet member? Shouldn't it get set or reset in this
method?
- What is the deal with the setCalendar() method? I am a little concerned
that we are throwing an UnsupportedOperationException. This is new?
- I don't see where AbsoluteTimeDateFormat(TimeZone) is deprecated. It
appears to be gone.
-----
ISO8601DateFormat.java
- I don't see where ISO8601DateFormat(TimeZone) is deprecated. It appears
to be gone.
-----
DateTimeDateFormat.java
- I don't see where DateTimeDateFormat(TimeZone) is deprecated. It appears
to be gone.
-----
PatternLayoutTestCase.java
- I got the following message reported when the test case started:
"D" is not a valid decimal seperator
Using Local defined decimal seperator
- Looking at test #15, the property file sets the layout to:
%d{DATE@cUS@len} [%t] %-5p %.16c - %m%n
but the witness file patternLayout.15 has messages that look like this:
[main] DEBUG rnLayoutTestCase - Message 0
Where is the date portion of the message? The witness file cannot be
correct, can it?
-----
AbsoluteTimeDateFormatTestCase.java
- I get the following error reported when the test case starts:
"X" is not a valid decimal seperator
etc
-----
tests/build.xml
- Somehow the SocketServer target had an arg value changed from 4 to 5.
This causes it to fail looking for a non-existent property file.
I still need to look at the test cases some more. I will do more on this
tomorrow.
Given the level and number of changes, I think these changes should be
applied to the 1.3 release, not the 1.2 release. I think these changes are
good, and the submission should be accepted pending my above comments and
the review of other committers.
-Mark
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
