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

Deprecate camel case date format #59555

Merged
merged 5 commits into from
Jul 21, 2020

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Jul 14, 2020

Camel case date formats are deprecated and snake case should be used
instead. An enmu FormatNames is introduced to keep pattern names for
both joda and java.time in one place

relates #58719

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

Camel case date formats are deprecated and snake case should be used
instead. An enmu FormatNames is introduced to keep pattern names for
both joda and java.time in one place
@pgomulka pgomulka requested a review from rjernst July 14, 2020 16:48
@pgomulka pgomulka self-assigned this Jul 14, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 14, 2020
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM. For the future, I think doing the deprecation inside the FormatNames class is a nasty side-effect of a boolean method. But I think it is ok for now, and can be removed again after this deprecation is backported.

@pgomulka
Copy link
Contributor Author

pgomulka commented Jul 15, 2020

@rjernst I agree - this indeed turned out to be the side effect on a boolean method.
I think I can quickly fix this just by adding a guard check in DateFormatters/Joda .forPattern method.

This approach means we would have to iterate twice over date formats. First to confirm if the format was camel case, then to return a predefined formatter. This could be refactored into a map of FormatNames->DateFormatter but I think in master we should aim to just reduce the number of predefined formatters first.

@pgomulka pgomulka merged commit c61dc11 into elastic:master Jul 21, 2020
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 21, 2020
Camel case date formats are deprecated and snake case should be used
instead. An enmu FormatNames is introduced to keep pattern names for
both joda and java.time in one place
pgomulka added a commit that referenced this pull request Jul 21, 2020
The method was accidentally added in #59555
Deprecation is being done in DateFormatters and Joda classes in guard pattern style.
pgomulka added a commit that referenced this pull request Jul 21, 2020
Camel case date formats are deprecated and snake case should be used
instead.
backports #59555
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jul 21, 2020
Camel case date formats are deprecated and snake case should be used
instead.
backports elastic#59555
pgomulka added a commit that referenced this pull request Jul 22, 2020
Camel case date formats are deprecated and snake case should be used
instead.
backports #59555 (#59948 from 7x)
pgomulka added a commit that referenced this pull request Jul 29, 2020
camel case named formats were deprecated since 7.9 and are removed in v8
relates #59555
WEEK_DATE("weekDate", "week_date"),
WEEK_DATE_TIME("weekDateTime", "week_date_time"),
WEEK_DATE_TIME_NO_MILLIS("weekDateTimeNoMillis", "week_date_time_no_millis"),
WEEK_YEAR("weekyear", "week_year"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@pgomulka The renaming of this to week_year seems inconsistent with the use of weekyear in other formats

WEEK_YEAR_WEEK("weekyearWeek", "weekyear_week"),
WEEKYEAR_WEEK_DAY("weekyearWeekDay", "weekyear_week_day"),

and

STRICT_WEEKYEAR("strictWeekyear", "strict_weekyear"),
STRICT_WEEKYEAR_WEEK("strictWeekyearWeek", "strict_weekyear_week"),
STRICT_WEEKYEAR_WEEK_DAY("strictWeekyearWeekDay", "strict_weekyear_week_day"),

Copy link
Member

Choose a reason for hiding this comment

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

To be clear, there isn't any rename in this PR. There is only bringing java formats inline with the already existing joda formats, which have this:

} else if ("weekyear".equals(input) || "week_year".equals(input)) {

I agree thought we should stick with weekyear, as that was not camelCase to begin with.

Copy link
Contributor

@russcam russcam Aug 5, 2020

Choose a reason for hiding this comment

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

Apologies, I don't think I was clear in my comment; I was referring to the enum ctor snakeCaseName parameter argument rather than the enum constant name (although the constant name WEEK_YEAR does look inconsistent). I came here because we have a failing test in the .NET client: elastic/elasticsearch-net#4920. It's related to #60044; thinking about it now, I should have opened it there I think 😄

russcam added a commit to russcam/elasticsearch that referenced this pull request Aug 5, 2020
Relates: elastic#60044 and elastic#59555 (review)

With the removal of camel case format names,
the format name for `WEEK_YEAR` has been set
to `week_year`, but this is inconsistent with the usage
in other formats related to weekyear e.g.

```
WEEKYEAR_WEEK("weekyear_week"),
WEEKYEAR_WEEK_DAY("weekyear_week_day"),
```

This commit

- replaces the snakecase format `week_year` with `weekyear`
- renames enum constants from `WEEK_YEAR` to `WEEKYEAR` for consistency
  with other constants related to weekyear.
russcam added a commit to russcam/elasticsearch that referenced this pull request Aug 7, 2020
Relates: elastic#60707

With the deprecation of camel case names in elastic#59555,
the format "weekyear" was deprecated in favour of
"week_year". This is inconsistent with other weekyear
formats however, that use "weekyear", for example

```
WEEKYEAR_WEEK("weekyear_week"),
WEEKYEAR_WEEK_DAY("weekyear_week_day"),
```

This commit keeps "weekyear" as the snake case name
for the enum constant FormatNames.WEEK_YEAR. In addition,
the format for DateFormatter WEEK_YEAR is changed to
"weekyear".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >deprecation Team:Core/Infra Meta label for core/infra team v7.8.2 v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants