Add NamedInstantPattern support to DatePatternConverter and tests#4128
Add NamedInstantPattern support to DatePatternConverter and tests#4128ashr123 wants to merge 4 commits into
NamedInstantPattern support to DatePatternConverter and tests#4128Conversation
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
Signed-off-by: Roy Ash <roy.ash456@gmail.com>
NamedInstantPattern support to DatePatternConverter and tests
|
@vy, @ppkarwasz 🙂 |
…tern Signed-off-by: Roy Ash <roy.ash456@gmail.com>
|
Thanks for this work and the continuation from #3789! The internal refactoring in DatePatternConverter looks much cleaner now. However, from a backward-compatibility and public API perspective, introducing NamedInstantPattern as a public enum is a heavy commitment. It permanently locks the public API into this specific Enum and prevents users from easily extending named patterns via plugins (since Enums are strictly final at compile-time). To achieve both your goal (making patterns programmatically accessible) and protecting the public API, I suggest a hybrid approach:
public final class InstantPatterns {
private InstantPatterns() {} // prevent instantiation
public static final String ABSOLUTE = "HH:mm:ss,SSS";
public static final String DEFAULT = "yyyy-MM-dd HH:mm:ss,SSS";
public static final String ISO8601 = "yyyy-MM-dd'T'HH:mm:ss,SSS";
// ...
}This approach gives users the programmatic access they need (InstantPatterns.ISO8601) while keeping our public API surface as flat and safe as possible." |
|
@ramanathan1504 the thing is that they can do it by themselves, inside their own projects for patterns that do not exist in this project… I don't think we need to create the "literals" twice The Enum will represent the "known" patterns, any project who wants to add patterns to it can create it's own constants/Enum and concatenate it into |
|
in addition, there is another way… but it is a bigger change… the user can optionally "supply" an enum ( |
|
@ashr123 I completely understand where you are coming from regarding avoiding duplication of the literals. It makes sense that users can just define their own custom patterns in their own codebase. My main concern with the public enum wasn't just about user extensibility, but rather the long-term API lock-in for the Log4j project. When we introduce a new public type (like an Enum) into org.apache.logging.log4j.core, it becomes a permanent contract. If the Log4j architecture changes in the future (for example, completely refactoring the date engine in 3.x to rely purely on java.time objects rather than string mapping), deprecating and removing a public Enum breaks backward compatibility. The beauty of exposing public static final String constants is that the Java compiler inlines the strings directly into the user's compiled .class files. This means Log4j provides the convenience of the pattern without strictly binding the framework's runtime API to the Enum structure forever. Regarding the Supplier idea—that's a very interesting concept for extensibility! But I agree with you that it might be a bit too heavy/over-engineered just for this specific PR. Since introducing new public API types is ultimately an architectural decision, perhaps we can leave the PR as it is for now and see what the core maintainers (like @pkarwasz or @vy ) prefer for the API surface? If they are comfortable committing to the public enum long-term, then your current approach is already perfectly clean! |
|
@ramanathan1504 see #3789 it is in version 2.26, if so, the current version (3.0.0) doesn't include those named patterns so it's now technically breaking compatibility I agree that if it will be pure private static HttpClient.Builder configureHttpClientVersion(HttpClient.Builder httpClientBuilder) {
try {
return httpClientBuilder.version(HttpClient.Version.valueOf("HTTP_3"));
} catch (IllegalArgumentException ignored) {
return httpClientBuilder;
}
}Again, compiled for Java 25, this will work on JRE 25 (it will use the default |
|
@ashr123 Thanks for the detailed explanation! I've been analyzing the whole picture based on our discussion, and I want to clarify my stance. We have a classic architectural trade-off here: For me, I am completely fine with either approach. Both have valid merits. Since this deals with the core public API structure, let's just wait for the core maintainers (like @ppkarwasz or @vy ) to weigh in. Whichever direction they align with—whether keeping the new class or nesting it in the old one—I will happily support it. Once they give the green light on the final structure, I'll gladly finish up the review and merge this. Thanks again for the great discussion and all your work on this! |
I've created an enum for named patterns, that way I think it will be easier to maintain.
In addition the enum modifier is public because it lets users to reuse those patterns in their application, this is. continuation to PR
NamedInstantPatternto make date & time patterns supported by Pattern Layout programmatically accessible #3789Checklist
2.xbranch if you are targeting Log4j 2; usemainotherwise./mvnw verifysucceeds (the build instructions)src/changelog/.2.x.xdirectory