Mike,
It is very rare to have non-committers to submit patches of this
breath. Including test cases in your contribution also deserves
credit. I join Mark in expressing my thanks. Thank you Mike.
As Mark observed these changes will go to 1.3 but possibly after
significant changes. In particular, I am uneasy about the %d suboption
syntax. %d{ISO8601@cCH@lit@d.@tCET} does not seem particularly user
friendly. Wouldn't it be possible to generalize option parsing for
*all* conversion specifiers?
Please also see also http://qos.ch/specs/PatternLayout.html.
Also, have you considered the effect of contradictory options? What would
%date{ISO8601@d!@tGMT@cUS@lit} result in (decimalSeparator=!,
timezone=GMT, country=US, language=italian)? Log4j avoids giving
users the possibility of specifying contradictory options.
Best regards,
At 23:35 16.07.2002 -0700, [EMAIL PROTECTED] wrote:
>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
--
Ceki
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
