Feature/2.x/enumerating and exposing named patters #3789
Conversation
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
There was a problem hiding this comment.
Hi @ashr123,
Thanks for the PR!
From my perspective, this refactoring does improve the readability of the decodeNamedPattern method—cleaner logic and better structure are always welcome. That said, readability can be subjective, so I'd also like to hear what @vy (the original author) thinks about the change.
Regarding the visibility of the new NamedPattern enum: as I mentioned in #3788 (reply in thread):
I'm not sure there's a strong case for making such an
enumpublic. Unless there's a clear user-facing need, we prefer to keep our API surface area minimal.
Would it be possible to make the NamedPattern enum package-private? Or would that conflict with the purpose of your change?
Currently, the PR triggers an API compatibility check failure:
[ERROR] Name Type Delta New Old Suggest If Prov.
[ERROR] * org.apache.logging.log4j.core.pattern PACKAGE MINOR 2.24.1 2.24.1 2.25.0 -
[ERROR] MINOR PACKAGE org.apache.logging.log4j.core.pattern
[ERROR] ADDED ENUM org.apache.logging.log4j.core.pattern.NamedPattern
This is caused by our BND Baseline mechanism, which flags any additions to the public API. If you do intend to make NamedPattern public, you'll need to update the @Version annotation in the package-info.java file for the org.apache.logging.log4j.core.pattern package. If not, making the enum package-private would avoid triggering this check.
Looking forward to your thoughts!
Generated by gradle/develocity-actions |
|
@ppkarwasz I see…
I'm also curious to find out what @vy thinks about it and I'll do whatever you decide |
There was a problem hiding this comment.
Disclosing legacy patterns in the binary footprint will make it impossible to deprecate the legacy support in the future. I'd appreciate it if you can update this class as follows:
- Rename this class to
DatePattern - Add minimal, to-the-point JavaDoc to the class definition. Please stick to the one-sentence-per-line convention we adhere to in our docs.
- Create a
private static final boolean COMPAT = InstantPatternFormatter.LEGACY_FORMATTERS_ENABLED;field and copy the entire documentation fromDatePatternConverterto here in Javadoc (i.e., in between/** ... */) - Make enum ctor to receive a single
Stringproperty (call itpatternin the class field):
...
ABSOLUTE_NANOS("HH:mm:ss," + (COMPAT ? "nnnnnnnnn" : "SSSSSSSSS")),
...
- Add a
public static String decode(final String pattern)method copy the associated JavaDoc fromDatePatternConverter. Don't build the logic on catchingIllegalArgumentException; use afor-loop overvalues(), orArrays.stream(values()).findFirst(...).map(...).orElse(pattern), etc. - Remove
DatePatternConverter#decodeNamedPattern(String) - Fix
DatePatternConverterandDatePatternConverterTestBase.java - Bump the
@Versioninpackage-info.javato2.26.0
There was a problem hiding this comment.
Disclosing legacy patterns in the binary footprint will make it impossible to deprecate the legacy support in the future. I'd appreciate it if you can update this class as follows:
(...)
8. Bump the@Versioninpackage-info.javato2.26.0
So should we expose the class (and bump the version) or hide the class from the public API. I am slightly pending towards hiding the class.
There was a problem hiding this comment.
I thought the whole point of this exercise is to allow users to programmatically access these constants, which are public API. If note, please help me understanding what we are doing here.
There was a problem hiding this comment.
@vy:
-
Java’s convention is to distinguish between date, time and date-time entities, if I’ll call it
DatePatternit may be a bit confusing, don’t you think? And indeed it concerns itself only about what you called named-patterns -
Regarding point 3, don’t you prefer using
InstantPatternFormatter.LEGACY_FORMATTERS_ENABLED(which is constant) directly instead of creating another constant inDatePattern? In addition, the compiler gives me the following error:Cannot read value of field 'COMPAT' before the field's definition
As constant fields are initialized after the literals
-
Regarding 5, the whole point is to use the built-in mechanism of mapping
Stringinto anEnum.. why should we iterate over the literals (and sort of re-inventing the wheel)?
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
I've created an enum for named patterns, that way I think it will be easier to maintain.
In addition the enum modifier is
publicbecause it lets users to reuse those patterns in their applicationChecklist
Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.
✅ Required checks
License: I confirm that my changes are submitted under the Apache License, Version 2.0.
Commit signatures: All commits are signed and verifiable. (See GitHub Docs on Commit Signature Verification).
Code formatting: The code is formatted according to the project’s style guide.
How to check and fix formatting
./mvnw spotless:check./mvnw spotless:applySee the build instructions for details.
Build & Test: I verified that the project builds and all unit tests pass.
How to build the project
Run:
./mvnw verifyI got the following error:
🧪 Tests (select one)
📝 Changelog (select one)
src/changelog/.2.x.x. (See Changelog Entry File Guide).