-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Use deprecation logger for CLDR date format specifiers #112917
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
Conversation
Pinging @elastic/es-core-infra (Team:Core/Infra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 👍
// check for all textual fields, and localized zone offset | ||
private static final Predicate<String> CONTAINS_CHANGING_TEXT_SPECIFIERS = System.getProperty("java.locale.providers", "") | ||
.contains("COMPAT") ? Pattern.compile("[EcGaO]|MMM|LLL|eee|ccc|QQQ|ZZZZ").asPredicate() : Predicates.never(); | ||
.contains("COMPAT") ? Pattern.compile("[BEGOavz]|LLL|MMM|QQQ|ccc|eee|Z{4}").asPredicate() : Predicates.never(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is B
? Should v
be upper case (time-zone ID)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have tests with these formats in DateUtilsTests to ensure we are checking/interpreting them correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B
is 'Period of day' ('in the morning'). v
is generic time-zone name (V
is time-zone id), which I've seen can be localised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Update some tests
b6ec7cb
to
190632d
Compare
@elasticmachine rerun elasticsearch-ci/7.17.25 |
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM
💚 Backport successful
|
The addition of the logger requires several updates to tests to deal with the possible warning, or muting if there is not way to specify an allowed (but not mandatory) warning
…3190) The addition of the logger requires several updates to tests to deal with the possible warning, or muting if there is not way to specify an allowed (but not mandatory) warning Co-authored-by: Elastic Machine <[email protected]>
Use the deprecation logger so warnings end up in Kibana.
Update regex for a few more textual specifiers.
I've had to modify or mute several tests to deal with the extra deprecation warning this outputs