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

Core: Add backcompat for joda time formats #36531

Merged
merged 2 commits into from Dec 13, 2018
Merged

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Dec 12, 2018

This commit adds deprecation warnings when using format specifiers with
joda data formats that will change with java time. It also adds the "8"
prefix which may be used to force the new java time format parsing.

This commit adds deprecation warnings when using format specifiers with
joda data formats that will change with java time. It also adds the "8"
prefix which may be used to force the new java time format parsing.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@rjernst
Copy link
Member Author

rjernst commented Dec 12, 2018

A couple things to note for reviewers:

  • locale was not needed in forPattern, it was always Locale.ROOT except for 2 tests
  • Technically the format "yyyy" is different in java time, but not for 99.9% of users. In java time this is year of era, but since using anything but the current era is so rare, this is effectively the same as just "year". I chose to not add a deprecation warning for that because it just added noise both in tests, and for users, for something that does not change in practice.

@spinscale
Copy link
Contributor

@elasticmachine retest this please

@spinscale
Copy link
Contributor

spinscale commented Dec 12, 2018

Did a first pass, will do another one later today (looks good so far).

On the danger of starting huge bike shedding: Do you think the 8 prefix is a good one? I think java8 is an implementation detail that noone needs to care about (and many of our non java users dont have an idea about that change). Would it make more sense to use 7 (for the major version upgrade) and also add an _ to make it easier to, as long as underscores are used instead of camelcase for the format name?

@spinscale
Copy link
Contributor

just to understand your argument above (seems my java time knowledge is fading again). The only time the remaining yyyy would be an issue, is when you are using that in combination with the weeks based dates, like yyyy'W'ww, correct?

@rjernst
Copy link
Member Author

rjernst commented Dec 12, 2018

Do you think the 8 prefix is a good one? I think java8 is an implementation detail that noone needs to care about

I think it is pertinent for users to know, in the same way they currently need to know it is joda time, so that they know what format specifiers are valid (by looking at the respective documentation).

The only time the remaining yyyy would be an issue, is when you are using that in combination with the weeks based dates, like yyyy'W'ww, correct?

I don't think week has anything to do with it. yyyy would be different than uuuu if you were printing dates before before year 0. For example, the year Julius caesar died would be -44 with uuuu and 44 with yyyy. It only comes up with looking at years in different eras.

Copy link
Contributor

@spinscale spinscale left a comment

Choose a reason for hiding this comment

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

LGTM! Docs are needed for a small mention about the new 8 prefixed format, but that can easily be done in a separate PR.

@rjernst
Copy link
Member Author

rjernst commented Dec 13, 2018

Thanks @spinscale. I will followup with docs in a separate PR.

@rjernst rjernst merged commit 254d1e8 into elastic:master Dec 13, 2018
@rjernst rjernst deleted the timeapi19 branch December 13, 2018 20:26
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Dec 13, 2018
This commit adds deprecation warnings when using format specifiers with
joda data formats that will change with java time. It also adds the "8"
prefix which may be used to force the new java time format parsing.
rjernst added a commit that referenced this pull request Dec 17, 2018
This commit adds deprecation warnings when using format specifiers with
joda data formats that will change with java time. It also adds the "8"
prefix which may be used to force the new java time format parsing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants