Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DateTimeFormatter constants to be used for the date pattern. #373

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jamezp
Copy link
Member

@jamezp jamezp commented Dec 20, 2022

Signed-off-by: James R. Perkins jperkins@redhat.com

@jamezp jamezp force-pushed the date-format branch 2 times, most recently from 2e0d1b3 to 8d22ca8 Compare December 20, 2022 18:27
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

StandardOutputStreams.printError("Failed to determine date constant %s. Defaulting to %s.", formatString, "yyyy-MM-dd HH:mm:ss,SSS");
result = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss,SSS");
}
CACHED_DATE_TIME_FORMATTERS.put(formatString, result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall seeing somewhere that ThreadLocal was used instead of a ConcurrentHashMap. Does that still exist? Is it a concern here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC a ThreadLocal was used because we used the SimpleDateFormatter which is not thread safe. The DateTimeFormatter should be thread safe, so think it should work. TBH we might not even need to cache it. It's likely not invoked that often at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I am asking, I don't know how often this is called :)

DateTimeFormatter result;
try {
final MethodHandle methodHandle = MethodHandles.publicLookup()
.findStaticGetter(DateTimeFormatter.class, formatString, DateTimeFormatter.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advise against using reflection for this. It would tie the supported formats to the JDK in use, which would probably cause an issue at some point if and when we want to support (generally speaking) a format that only exists by name in a later JDK.

Also, the cache is probably overkill given that patterns are seldom created; I think a String-switch which resolves a name to the corresponding string or predefined constant is probably best. This allows us to control the namespace and also support formats that do not exist by name in the API, and also provides protection if such a format is introduced under a different name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. That is a valid point. I was trying to think of a generic way to handle it, but it makes sense to keep it hard-coded.

@x80486
Copy link

x80486 commented Apr 23, 2023

Hello everyone! Is this one ready or is it waiting for something else?

@jamezp
Copy link
Member Author

jamezp commented Apr 24, 2023

@x80486 It's on a temporary hold.

@jamezp jamezp marked this pull request as ready for review May 8, 2023 20:45
@@ -477,8 +477,7 @@ public static FormatStep dateFormatStep(final TimeZone timeZone, final String fo
final int minimumWidth,
final boolean truncateBeginning, final int maximumWidth) {
return new JustifyingFormatStep(leftJustify, minimumWidth, truncateBeginning, maximumWidth) {
final DateTimeFormatter dtf = DateTimeFormatter
.ofPattern(formatString == null ? "yyyy-MM-DD HH:mm:ss,SSS" : formatString);
final DateTimeFormatter dtf = StandardDateFormat.resolve(formatString);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if instead of the enum, you did something along these lines in (half-Java, half-pseudocode):

private DateTimeFormatter resolve(String formatString) {
    if (formatString.length() > 1 && formatString.charAt(0) == '#') {
        formatString = formatString.substring(1);
        if (formatString.charAt(0) != '#') { // double-## means pass thru
            switch (formatString) {
                case "ISO_DATE": return DateTimeFormatter.ISO_DATE;
                // ... and one case for each known string that we want to allow ...
                default: return DateTimeFormatter.ofPattern("'<Unknown format " + formatString + ">'");
            }
        }
    }
    return DateTimeFormatter.ofPattern(formatString);
}

This keeps the code minimal and simple, is guaranteed to be compatible with future DateTimeFormatter utilizations of #, and is forward-compatible as well in case we want to support fancier things like putting the format name in mid-string or whatever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the text format would be something like %d{#ISO_DATE}?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. This way we never need to be concerned about syntactic conflicts between constant names and format strings.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, that character really has no meaningful use from the user standpoint, with the exception of identifying, by convention, date/time pattern formats from constants — something you can achieve performing proper parsing.

I find it more confusing, in the sense that single characters in that particular string (e.g.: %date{ISO8601} |- %highlight(%5level) in %cyan(%logger{32}:%line) %magenta([%t - %m%n%ex) have a particular meaning.

Still it's a minor detail, but it amounts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@x80486 The main reason for the prefix character is most characters are reserved for a DateTimePattern. While ISO_DATE or ISO08601 is likely not going to bean issue, it could be a valid pattern at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants