vy commented on code in PR #3789:
URL: https://github.com/apache/logging-log4j2/pull/3789#discussion_r2196798155
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/DatePatternConverter.java:
##########
@@ -109,40 +109,19 @@ private static String readPattern(@Nullable final
String[] options) {
* @since 2.25.0
*/
static String decodeNamedPattern(final String pattern) {
- // If legacy formatters are enabled, we need to produce output aimed
for `FixedDateFormat` and `FastDateFormat`.
Review Comment:
> Can we simplify this comment a bit? It currently takes many lines to
explain two main points:
>
> * `FixedDateFormat` uses `n` to represent various sub-second fractions,
while `DateTimeFormatter` uses `n` strictly for nanoseconds.
> * `X` in `DateTimeFormatter` returns `"Z"` for zero offsets, whereas
`SimpleDateFormat` returns `"00:00"`.
The comment block does not only explain how things work the way they are –
this is what your simplification implies. The comment block _additionally_
shares the historical context, _why_ things work the way they are. Not to
mention, it adds several valuable historical and contextual information for
those who are not experts in the subject. I want to leave the code in a state
that a developer who has not even heard about `FDF`, `SDF`, `DTF`, etc. can
quickly have an overview to tackle any potential bugs. I am really adamant
about keeping it intact, since it was key in my date & time fixes while still
trying to be backward compatible.
> At the same time, it omits some helpful context — for example, why do we
convert `ABSOLUTE_PERIOD` to `"HH:mm:ss.SSS"` instead of passing it directly to
the legacy formatter?
The legacy formatter is, as its name implies, legacy, and it shall not be
used. It has several bugs, still. The whole point of this exercise was to stop
using it. Your questions is analogous to _"Why do new JDK classes don't use
`Vector`, `Stack`, and `Hashtable` anymore? This is not explained anywhere."_
But if this is not clear for you, I am okay with explicitly pointing this
detail out. As a matter of fact, this is stated already in the
`decodeNamedPattern` Javadoc:
```
* In version {@code 2.25.0}, {@link FixedDateFormat} and {@link
FastDateFormat} are deprecated in favor of {@link InstantPatternFormatter}.
* We introduced this method to keep backward compatibility with the
named patterns provided by {@link FixedDateFormat}.
```
If you feel the need, you can enhance the existing comment block with such a
preamble. But, can we keep the existing details, without omission, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
