Set start of the week to Monday for root locale #43652
Conversation
|
Pinging @elastic/es-core-infra |
| package org.elasticsearch.common.time; | ||
|
|
||
| public class RLocale { | ||
| public static final java.util.Locale INS = new java.util.Locale.Builder() |
There was a problem hiding this comment.
Can we put this in DateUtils instead of creating a new class? I also don't know what "INS" is supposed to mean. Maybe ISO8601_LOCALE?
There was a problem hiding this comment.
RLocale.INS was same length as Locale.ROOT - just wanted to quickly see if this passes CI and didn't want to fix all line length violations.
The name obviously ha to be changed - ISO8601_LOCALE sounds good to me.
| public class RLocale { | ||
| public static final java.util.Locale INS = new java.util.Locale.Builder() | ||
| .setLocale(java.util.Locale.ROOT) | ||
| .setUnicodeLocaleKeyword("fw", "mon").build(); |
There was a problem hiding this comment.
Please add a comment explaining why this is set to monday, and linking to the appropriate java discussion/code/iso definition.
| @@ -24,6 +23,8 @@ | |||
| import static java.time.temporal.ChronoField.SECOND_OF_MINUTE; | |||
|
|
|||
| public final class StringUtils { | |||
| public static final java.util.Locale WEEK_START_MON = new java.util.Locale.Builder() | |||
There was a problem hiding this comment.
Why another copy? Can we use a single shared locale with this set to monday?
There was a problem hiding this comment.
this was here because the sql project is not depending on server
There was a problem hiding this comment.
Ok, then maybe do put it into it's own class, but in core/libs? It is all jdk based stuff so it should be fine there.
|
@rjernst this is still very much a draft, I will certainly rework this properly once I am back from support ramp up. |
pgomulka
changed the title
|
@rjernst this is ready for a review now. I wanted also to include Locale.ROOT into forbidden api but there is around 2000usages of that in places which are not related to dates (String.format or toLowerCase methods) |
| * Locale constants to be used across elasticsearch code base. | ||
| * java.util.Locale.ROOT should not be used as it defaults start of the week incorrectly to Sunday. | ||
| */ | ||
| public class Locale { |
There was a problem hiding this comment.
Can we use a different class name that does not collide with java's Locale, so that we don't need to deal with fully qualified class names? Perhaps IsoLocale (we should also make this class not constructable).
|
@elasticmachine run elasticsearch-ci/default-distro |
|
@elasticmachine run elasticsearch-ci/1 |
Introducing a IsoLocal.ROOT constant which should be used instead of java.util.Locale.ROOT in ES when dealing with dates. IsoLocal.ROOT customises start of the week to be Monday instead of Sunday. closes elastic#42588 an issue with investigation details relates elastic#41670 bug raised (this won't fix it on its own. joda.parseInto has to be reimplemented closes elastic#43275 an issue raised by community member
|
@pgomulka I change the version label here to 7.5 while preparing the 7.4 release notes since the backport is still pending and this is an enhancement. Please let me know if this is wrong |
This reverts commit 8d1ea86.
Introducing a IsoLocal.ROOT constant which should be used instead of java.util.Locale.ROOT in ES when dealing with dates. IsoLocal.ROOT customises start of the week to be Monday instead of Sunday. closes elastic#42588 an issue with investigation details relates elastic#41670 bug raised (this won't fix it on its own. joda.parseInto has to be reimplemented closes elastic#43275 an issue raised by community member change skip reason compile error not orking spi working unit test cleanup change providers for 9+ revert changes IsoLocale cleanup move spi files to server make unit test pass from gradle expermienting with gradle tasks uncomment jar hell check only add settings in buildplugin allign options for locale providers
initial work for simplest approach to set start of the week to monday
other could be using using custom locale provider (extending CLDR and overriding startoftheweek method)
relates #42588 an issue with investigation details
relates #41670 bug raised (this won't fix it on its own. joda.parseInto has to be reimplemented
relates #43275 fixed