You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/07/20 10:47:56 UTC
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu opened a new pull request #11: SLING-9989: migrate date formating to java.time API
Buuhuu opened a new pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884036226
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673791431
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
I consider `CLDR` the more compatible solution across different JDK distributions, therefore please rely on that whenever possible and set that as preferred option via https://docs.oracle.com/javase/8/docs/api/java/util/Properties.html#setProperty-java.lang.String-java.lang.String-. That way it should work both from IDE and from Maven. As long as tests are not executed in parallel it should be safe to set to `CLDR` prior to your tests and reset it after them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673831412
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
When using `CLDR` the tests work on oracle jdk8 and fail on oracle jdk11 and any other jdk11. If using `COMPAT` works for both, so imho we should keep it like that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673836303
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
It was wrong in the JDK8 I guess. Btw not sure if it is by intent but Jenkins builds with jdk8 and runs sonar with jdk11.
short / GMT+02:00 / fr
==> jdk8u301 `01/12/18`
==> jdk11.0.12 `01/12/1918`
EEEE, d MMM y / UTC / de
==> jdk8u301 `Sonntag, 1 Dez 1918`
==> jdk11.0.12 `Sonntag, 1 Dez. 1918`
dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w / UTC
==> jdk8u301 `01 December '18 12:00 AM; day in year: 335; week in year: 49`
==> jdk11.0.12 `01 December '18 12:00 AM; day in year: 335; week in year: 48`
As said those examples are taken from the htl spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-883950628
I had to ignore some tests as they cause ambiguous results depending on the jdk version (prior jdk8 behaves differently then jdk11) and jdk implementations (oracle jdk behaves differently then openjdk). The reason is the change of the [Locale Data Providers](https://www.oracle.com/java/technologies/javase/jdk11-suported-locales.html) which with jdk >= 9 has CLDR but maybe in different versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673696266
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -176,47 +187,39 @@ private String toString(RuntimeObjectModel runtimeObjectModel, Object[] params,
return "";
}
- private int getPredefinedFormattingStyleFromValue(String value) {
+ private FormatStyle getPredefinedFormattingStyleFromValue(String value) {
switch (value.toLowerCase(Locale.ROOT)) {
case "default":
- return DateFormat.DEFAULT;
- case "short":
- return DateFormat.SHORT;
case "medium":
- return DateFormat.MEDIUM;
+ return FormatStyle.MEDIUM;
+ case "short":
+ return FormatStyle.SHORT;
case "long":
- return DateFormat.LONG;
+ return FormatStyle.LONG;
case "full":
- return DateFormat.FULL;
+ return FormatStyle.FULL;
default:
- return -1;
+ return null;
}
}
- private String formatDate(String format, Date date, Locale locale, TimeZone timezone) {
+ private String formatDate(String format, Date date, Locale locale, TimeZone timezone, DateTimeFormatter formatter) {
if (date == null) {
return null;
}
try {
- final DateFormat formatter;
- int formattingStyle = getPredefinedFormattingStyleFromValue(format);
- if (formattingStyle != -1) {
- if (locale != null) {
- formatter = DateFormat.getDateInstance(formattingStyle, locale);
+ if (formatter == null) {
Review comment:
This was a leftover of a wip commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673836303
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
It was wrong/outdated in the JDK8 I guess. Btw not sure if it is by intent but Jenkins builds with jdk8 and runs sonar with jdk11.
short / GMT+02:00 / fr
==> jdk8u301 `01/12/18`
==> jdk11.0.12 `01/12/1918`
EEEE, d MMM y / UTC / de
==> jdk8u301 `Sonntag, 1 Dez 1918`
==> jdk11.0.12 `Sonntag, 1 Dez. 1918`
dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w / UTC
==> jdk8u301 `01 December '18 12:00 AM; day in year: 335; week in year: 49`
==> jdk11.0.12 `01 December '18 12:00 AM; day in year: 335; week in year: 48`
As said those examples are taken from the htl spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673013339
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
I am not sure how this branch can be reached, if it is dead code we should remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884793443
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [87.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673013339
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
I am not sure how this branch can be reached, if it is dead code we should remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673800542
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
COMPAT is used here as well https://github.com/apache/sling-org-apache-sling-scripting-sightly-testing/blob/09b67343e22d06db7cbe01e8c3e8342df1d01d63/pom.xml#L194
It should actually be `COMPAT,SPI` according to https://www.oracle.com/java/technologies/javase/java9locales.html
> Specifying the system property with "COMPAT,SPI" will have the same behavior with the prior JDK releases.
`CLDR` is not reliable as it depends on the version used by the particular implementation java.net jdk11.0.2 gives different results then the oracle jdk11 gives.
The configuration cannot be changed at runtime
> The search order of locale sensitive services can be configured by using the "java.locale.providers" system property. This system property declares the user's preferred order for looking up the locale sensitive services separated by a comma. It is only read at the Java runtime startup, so the later call to System.setProperty() won't affect the order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] raducotescu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673039030
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
I'm afraid I don't understand the question. The code before attempted to check if the passed string was a valid date format pattern, which is similar to what the new code does.
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
Ah, that's simple - you have no `type` option passed along the `format` option, so we have to figure out what kind of pattern the user provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673036051
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
@raducotescu Can you answer this?
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -176,47 +187,39 @@ private String toString(RuntimeObjectModel runtimeObjectModel, Object[] params,
return "";
}
- private int getPredefinedFormattingStyleFromValue(String value) {
+ private FormatStyle getPredefinedFormattingStyleFromValue(String value) {
switch (value.toLowerCase(Locale.ROOT)) {
case "default":
- return DateFormat.DEFAULT;
- case "short":
- return DateFormat.SHORT;
case "medium":
- return DateFormat.MEDIUM;
+ return FormatStyle.MEDIUM;
+ case "short":
+ return FormatStyle.SHORT;
case "long":
- return DateFormat.LONG;
+ return FormatStyle.LONG;
case "full":
- return DateFormat.FULL;
+ return FormatStyle.FULL;
default:
- return -1;
+ return null;
}
}
- private String formatDate(String format, Date date, Locale locale, TimeZone timezone) {
+ private String formatDate(String format, Date date, Locale locale, TimeZone timezone, DateTimeFormatter formatter) {
if (date == null) {
return null;
}
try {
- final DateFormat formatter;
- int formattingStyle = getPredefinedFormattingStyleFromValue(format);
- if (formattingStyle != -1) {
- if (locale != null) {
- formatter = DateFormat.getDateInstance(formattingStyle, locale);
+ if (formatter == null) {
Review comment:
where is this private method called with formatter != null? Why do we need that parameter?
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
The question is rather under which circumstances do you end in this area as there are 4 return statements before this line (which IMHO cover all configuration options...)
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
@Buuhuu Can you make sure this is covered by your unit test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884062355
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [85.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] raducotescu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673039030
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
I'm afraid I don't understand the question. The code before attempted to check if the passed string was a valid date format pattern, which is similar to what the new code does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r674114826
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
I did also some more tests, JDK8 (Zulu, AdoptOpenJdk and Oracle) ships with CLDR v27, while JDK11 ships with CLDR v33 (again all three distributions). The differences are quite fundamental (surprisingly) for instance https://bugs.openjdk.java.net/browse/JDK-8211262 or the year using 4 digits instead of 2 in short format style for most locales.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673836303
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
It was wrong in the JDK8 I guess. Btw not sure if it is intent but Jenkins builds with jdk8 and runs sonar with jdk11.
short / GMT+02:00 / fr
==> jdk8u301 `01/12/18` vs jdk11.0.12 `01/12/1918`
EEEE, d MMM y / UTC / de
==> jdk8u301 `Sonntag, 1 Dez 1918` vs jdk11.0.12 `Sonntag, 1 Dez. 1918`
dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w / UTC
==> jdk8u301 `01 December '18 12:00 AM; day in year: 335; week in year: 49`
==> jdk11.0.12 `01 December '18 12:00 AM; day in year: 335; week in year: 48`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] bdelacretaz commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-883962064
> ...I had to ignore some tests as they cause ambiguous results depending on the jdk version...
I think we need to find a way to have stable tests, I haven't used [CLDR](https://openjdk.java.net/jeps/252) myself so far but that might be a good option. And briefly describe that solution in SLING-9989 to make it easier to find that information later if we need it, as I suppose we might encounter the same problem elsewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673785311
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
Yes setting it explicitly to *COMPAT* would work, but only when you run the tests from maven. If you run them in the ide you have to configure the test runner as well accordingly. It should be good enough to leave a comment at the tests for that case.
Please also see my comment above: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-883950628
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884125527
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [87.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673721777
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
Isn't this rather depending on the Locale Provider? If you set this explicitly to CLDR (https://openjdk.java.net/jeps/252) it should work with all distributions IMHO, compare with https://issues.apache.org/jira/browse/SLING-9984 and https://docs.huihoo.com/java/javase/9/docs/api/java/util/spi/LocaleServiceProvider.html.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673836303
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
It was wrong in the JDK8 I guess. Btw not sure if it is by intent but Jenkins builds with jdk8 and runs sonar with jdk11.
short / GMT+02:00 / fr
==> jdk8u301 `01/12/18` vs jdk11.0.12 `01/12/1918`
EEEE, d MMM y / UTC / de
==> jdk8u301 `Sonntag, 1 Dez 1918` vs jdk11.0.12 `Sonntag, 1 Dez. 1918`
dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w / UTC
==> jdk8u301 `01 December '18 12:00 AM; day in year: 335; week in year: 49`
==> jdk11.0.12 `01 December '18 12:00 AM; day in year: 335; week in year: 48`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] removed a comment on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884036226
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673037597
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -176,47 +187,39 @@ private String toString(RuntimeObjectModel runtimeObjectModel, Object[] params,
return "";
}
- private int getPredefinedFormattingStyleFromValue(String value) {
+ private FormatStyle getPredefinedFormattingStyleFromValue(String value) {
switch (value.toLowerCase(Locale.ROOT)) {
case "default":
- return DateFormat.DEFAULT;
- case "short":
- return DateFormat.SHORT;
case "medium":
- return DateFormat.MEDIUM;
+ return FormatStyle.MEDIUM;
+ case "short":
+ return FormatStyle.SHORT;
case "long":
- return DateFormat.LONG;
+ return FormatStyle.LONG;
case "full":
- return DateFormat.FULL;
+ return FormatStyle.FULL;
default:
- return -1;
+ return null;
}
}
- private String formatDate(String format, Date date, Locale locale, TimeZone timezone) {
+ private String formatDate(String format, Date date, Locale locale, TimeZone timezone, DateTimeFormatter formatter) {
if (date == null) {
return null;
}
try {
- final DateFormat formatter;
- int formattingStyle = getPredefinedFormattingStyleFromValue(format);
- if (formattingStyle != -1) {
- if (locale != null) {
- formatter = DateFormat.getDateInstance(formattingStyle, locale);
+ if (formatter == null) {
Review comment:
where is this private method called with formatter != null? Why do we need that parameter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] removed a comment on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884059330
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [85.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673800542
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
COMPAT is used here as well https://github.com/apache/sling-org-apache-sling-scripting-sightly-testing/blob/09b67343e22d06db7cbe01e8c3e8342df1d01d63/pom.xml#L194
It should actually be `COMPAT,SPI` according to https://www.oracle.com/java/technologies/javase/java9locales.html
> Specifying the system property with "COMPAT,SPI" will have the same behavior with the prior JDK releases.
`CLDR` is not reliable as it depends on the version used by the particular implementation java.net jdk11.0.2 gives different results then the oracle jdk11 gives.
The configuration cannot be changed on runtime
> The search order of locale sensitive services can be configured by using the "java.locale.providers" system property. This system property declares the user's preferred order for looking up the locale sensitive services separated by a comma. It is only read at the Java runtime startup, so the later call to System.setProperty() won't affect the order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r674134187
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -0,0 +1,178 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ ******************************************************************************/
+package org.apache.sling.scripting.sightly.impl.engine.extension;
+
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+
+import org.apache.sling.scripting.sightly.SightlyException;
+import org.apache.sling.scripting.sightly.render.AbstractRuntimeObjectModel;
+import org.apache.sling.scripting.sightly.render.RenderContext;
+import org.apache.sling.scripting.sightly.render.RuntimeObjectModel;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+public class FormatFilterExtensionTest {
+
+ private final RenderContext renderContext = new RenderContext() {
+ @Override public RuntimeObjectModel getObjectModel() {
+ return new AbstractRuntimeObjectModel() {
+ };
+ }
+
+ @Override public Bindings getBindings() {
+ return new SimpleBindings();
+ }
+
+ @Override public Object call(String s, Object... objects) {
+ return null;
+ }
+ };
+ private final FormatFilterExtension subject = new FormatFilterExtension();
+ private final Date testDate = Date.from(LocalDateTime.of(1918, 12, 1, 0, 0, 0, 0)
+ .atZone(ZoneId.of("UTC"))
+ .toInstant());
+
+ @Test
+ public void testDateFormatNull() {
+ assertNull(subject.call(renderContext, "default", new HashMap<String, Object>() {{
+ put(FormatFilterExtension.TYPE_OPTION, "date");
+ put(FormatFilterExtension.FORMAT, null);
+ }}));
+ }
+
+ @Test
+ public void testDateFormatNoDateObject() {
+ assertNull(subject.call(renderContext, "yyyy-MM-dd", Collections.singletonMap(FormatFilterExtension.FORMAT, new Object())));
+ }
+
+ @Test(expected = SightlyException.class)
+ public void testDateFormatFalseFormat() {
+ assertDate(null, "yT", null, null);
+ }
+
+ @Test
+ public void testDateFormatWithUTC() {
+ assertDate("1918-12-01 00:00:00.000Z", "yyyy-MM-dd HH:mm:ss.SSSXXX", "UTC", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneOffset() {
+ assertDate("1918-12-01 02:00:00.000+02:00", "yyyy-MM-dd HH:mm:ss.SSSXXX", "GMT+02:00", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneOffsetRFC822() {
+ assertDate("1918-12-01 02:00:00.000+0200", "yyyy-MM-dd HH:mm:ss.SSSZ", "GMT+02:00", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneName() {
+ assertDate("1918-12-01 02:00:00.000(GMT+02:00)", "yyyy-MM-dd HH:mm:ss.SSS(z)", "GMT+02:00", null);
+ }
+
+ /**
+ * When using jdk9 or newer, make sure to set the {@code java.locale.providers = COMPAT,SPI}
+ *
+ * @see <a href="https://docs.oracle.com/javase/9/docs/api/java/util/spi/LocaleServiceProvider.html">LocaleServiceProvider</a>
+ */
+ @Test
+ public void testDateFormatWithEscapedCharacters() {
+ assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
Review comment:
Can we work with https://junit.org/junit4/javadoc/4.12/org/junit/Assume.html here to make sure that COMPAT is being active? Probably this requires a check for either `System.getProperty("java.version")` smaller than 9 or `System.getProperty("java.locale.providers") starting with "COMPAT". WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] raducotescu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
raducotescu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673041882
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
Ah, that's simple - you have no `type` option passed along the `format` option, so we have to figure out what kind of pattern the user provided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673808118
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
Maybe we should add the change to the surefire plugin configuration to the parent pom https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11/commits/586ff8048c578d0ac6aa2e632f4745bc2294e746. I used COMPAT,JRE,SPI to be compatible to jdk8 and jdk9+
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu edited a comment on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu edited a comment on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-883950628
I had to ignore some tests as they cause ambiguous results depending on the jdk version (prior jdk8 behaves differently then jdk11) and jdk implementations (oracle jdk behaves differently then openjdk). The reason is the change of the [Locale Data Providers](https://www.oracle.com/java/technologies/javase/jdk11-suported-locales.html) which with jdk >= 9 uses CLDR as default, but maybe in different versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r674639402
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -0,0 +1,178 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ ******************************************************************************/
+package org.apache.sling.scripting.sightly.impl.engine.extension;
+
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+
+import org.apache.sling.scripting.sightly.SightlyException;
+import org.apache.sling.scripting.sightly.render.AbstractRuntimeObjectModel;
+import org.apache.sling.scripting.sightly.render.RenderContext;
+import org.apache.sling.scripting.sightly.render.RuntimeObjectModel;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+public class FormatFilterExtensionTest {
+
+ private final RenderContext renderContext = new RenderContext() {
+ @Override public RuntimeObjectModel getObjectModel() {
+ return new AbstractRuntimeObjectModel() {
+ };
+ }
+
+ @Override public Bindings getBindings() {
+ return new SimpleBindings();
+ }
+
+ @Override public Object call(String s, Object... objects) {
+ return null;
+ }
+ };
+ private final FormatFilterExtension subject = new FormatFilterExtension();
+ private final Date testDate = Date.from(LocalDateTime.of(1918, 12, 1, 0, 0, 0, 0)
+ .atZone(ZoneId.of("UTC"))
+ .toInstant());
+
+ @Test
+ public void testDateFormatNull() {
+ assertNull(subject.call(renderContext, "default", new HashMap<String, Object>() {{
+ put(FormatFilterExtension.TYPE_OPTION, "date");
+ put(FormatFilterExtension.FORMAT, null);
+ }}));
+ }
+
+ @Test
+ public void testDateFormatNoDateObject() {
+ assertNull(subject.call(renderContext, "yyyy-MM-dd", Collections.singletonMap(FormatFilterExtension.FORMAT, new Object())));
+ }
+
+ @Test(expected = SightlyException.class)
+ public void testDateFormatFalseFormat() {
+ assertDate(null, "yT", null, null);
+ }
+
+ @Test
+ public void testDateFormatWithUTC() {
+ assertDate("1918-12-01 00:00:00.000Z", "yyyy-MM-dd HH:mm:ss.SSSXXX", "UTC", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneOffset() {
+ assertDate("1918-12-01 02:00:00.000+02:00", "yyyy-MM-dd HH:mm:ss.SSSXXX", "GMT+02:00", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneOffsetRFC822() {
+ assertDate("1918-12-01 02:00:00.000+0200", "yyyy-MM-dd HH:mm:ss.SSSZ", "GMT+02:00", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneName() {
+ assertDate("1918-12-01 02:00:00.000(GMT+02:00)", "yyyy-MM-dd HH:mm:ss.SSS(z)", "GMT+02:00", null);
+ }
+
+ /**
+ * When using jdk9 or newer, make sure to set the {@code java.locale.providers = COMPAT,SPI}
+ *
+ * @see <a href="https://docs.oracle.com/javase/9/docs/api/java/util/spi/LocaleServiceProvider.html">LocaleServiceProvider</a>
+ */
+ @Test
+ public void testDateFormatWithEscapedCharacters() {
+ assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
Review comment:
Makes sense, yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673785311
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
Yes setting it explicitly to *COMPAT* would work, but only when you run the tests from maven. If you run them in the ide you have to configure the test runner as well accordingly. It should be good enough to leave a comment at the tests for that case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r674134187
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -0,0 +1,178 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ ******************************************************************************/
+package org.apache.sling.scripting.sightly.impl.engine.extension;
+
+import java.time.LocalDateTime;
+import java.time.ZoneId;
+import java.util.Collections;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.script.Bindings;
+import javax.script.SimpleBindings;
+
+import org.apache.sling.scripting.sightly.SightlyException;
+import org.apache.sling.scripting.sightly.render.AbstractRuntimeObjectModel;
+import org.apache.sling.scripting.sightly.render.RenderContext;
+import org.apache.sling.scripting.sightly.render.RuntimeObjectModel;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
+
+public class FormatFilterExtensionTest {
+
+ private final RenderContext renderContext = new RenderContext() {
+ @Override public RuntimeObjectModel getObjectModel() {
+ return new AbstractRuntimeObjectModel() {
+ };
+ }
+
+ @Override public Bindings getBindings() {
+ return new SimpleBindings();
+ }
+
+ @Override public Object call(String s, Object... objects) {
+ return null;
+ }
+ };
+ private final FormatFilterExtension subject = new FormatFilterExtension();
+ private final Date testDate = Date.from(LocalDateTime.of(1918, 12, 1, 0, 0, 0, 0)
+ .atZone(ZoneId.of("UTC"))
+ .toInstant());
+
+ @Test
+ public void testDateFormatNull() {
+ assertNull(subject.call(renderContext, "default", new HashMap<String, Object>() {{
+ put(FormatFilterExtension.TYPE_OPTION, "date");
+ put(FormatFilterExtension.FORMAT, null);
+ }}));
+ }
+
+ @Test
+ public void testDateFormatNoDateObject() {
+ assertNull(subject.call(renderContext, "yyyy-MM-dd", Collections.singletonMap(FormatFilterExtension.FORMAT, new Object())));
+ }
+
+ @Test(expected = SightlyException.class)
+ public void testDateFormatFalseFormat() {
+ assertDate(null, "yT", null, null);
+ }
+
+ @Test
+ public void testDateFormatWithUTC() {
+ assertDate("1918-12-01 00:00:00.000Z", "yyyy-MM-dd HH:mm:ss.SSSXXX", "UTC", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneOffset() {
+ assertDate("1918-12-01 02:00:00.000+02:00", "yyyy-MM-dd HH:mm:ss.SSSXXX", "GMT+02:00", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneOffsetRFC822() {
+ assertDate("1918-12-01 02:00:00.000+0200", "yyyy-MM-dd HH:mm:ss.SSSZ", "GMT+02:00", null);
+ }
+
+ @Test
+ public void testDateFormatWithZoneName() {
+ assertDate("1918-12-01 02:00:00.000(GMT+02:00)", "yyyy-MM-dd HH:mm:ss.SSS(z)", "GMT+02:00", null);
+ }
+
+ /**
+ * When using jdk9 or newer, make sure to set the {@code java.locale.providers = COMPAT,SPI}
+ *
+ * @see <a href="https://docs.oracle.com/javase/9/docs/api/java/util/spi/LocaleServiceProvider.html">LocaleServiceProvider</a>
+ */
+ @Test
+ public void testDateFormatWithEscapedCharacters() {
+ assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
Review comment:
Can we work with https://junit.org/junit4/javadoc/4.12/org/junit/Assume.html here to make sure that COMPAT is being active? Probably this requires a check for either `System.getProperty("java.version")` smaller than 9 or `System.getProperty("java.locale.providers")` starting with "COMPAT". WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673036051
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
@raducotescu Can you answer this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673808118
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
Maybe we should add the change to the surefire plugin configuration to the parent pom https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11/commits/586ff8048c578d0ac6aa2e632f4745bc2294e746
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r674659334
##########
File path: pom.xml
##########
@@ -54,13 +54,21 @@
<properties>
<sling.java.version>8</sling.java.version>
+ <java.locale.providers>COMPAT,JRE,SPI</java.locale.providers>
Review comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673695624
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
But only for the condition that the `format` option is not one of `Date` or `Calendar`, in which case the `getDateFormattedString()` always returns null https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11/commits/7e43ff55a4d27c89a35fa0041f9b97534881e093
I cannot find anything in the spec about this behaviour but it seems reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673811486
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
> COMPAT is used here as well https://github.com/apache/sling-org-apache-sling-scripting-sightly-testing/blob/09b67343e22d06db7cbe01e8c3e8342df1d01d63/pom.xml#L194
This is for executing the HTL TCK (https://github.com/adobe/htl-tck) which cannot be changed easily. You are right that this change only fixes compatibility with Oracle JDKs If you require the TCP with being compatible with other JDKs then it is much more effort.
For local Sling test we are free to define Tests in a more compatible way which is IMHO CLDR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] removed a comment on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-883951385
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673723824
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
We rely on stable output also in the HTL Integration tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673831412
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
When using `CLDR` the tests work on oracle jdk8 and fail on oracle jdk11 and any other jdk11. If using `COMPAT` works for both, so imho we should keep it like that.
Also I think for a user looking at the tests it makes more sense to see the same expected values as they are in the implemented specification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673811486
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
> COMPAT is used here as well https://github.com/apache/sling-org-apache-sling-scripting-sightly-testing/blob/09b67343e22d06db7cbe01e8c3e8342df1d01d63/pom.xml#L194
This is for executing the HTL TCK (https://github.com/adobe/htl-tck) which cannot be changed easily. You are right that this change only fixes compatibility with Oracle JDKs If you require the TCP with being compatible with other JDKs then it is much more effort.
For local Sling test we are free to define Tests in a more compatible way which is IMHO CLRD.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] bdelacretaz commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673787887
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
If the tests run reliably from Maven I'm fine, and as you mention a comment for IDEs is good enough. But I don't think the tests should be @ignored by default, my view is that Maven must run all tests reliably, and IDEs come second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r674116005
##########
File path: pom.xml
##########
@@ -54,13 +54,21 @@
<properties>
<sling.java.version>8</sling.java.version>
+ <java.locale.providers>COMPAT,JRE,SPI</java.locale.providers>
Review comment:
Would you mind adding a comment that CLDR changed drastically between JDK8 and 11?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] removed a comment on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884062355
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [85.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673695624
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
But only for the condition that the `format` option is not one of `Date` or `Calendar` (... a date according to `org.apache.sling.scripting.sightly.render.RuntimeObjectModel#isDate`), in which case the `getDateFormattedString()` always returns null https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11/commits/7e43ff55a4d27c89a35fa0041f9b97534881e093
I cannot find anything in the spec about this behaviour but it seems reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673832995
##########
File path: src/test/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtensionTest.java
##########
@@ -99,47 +100,26 @@ public void testDateFormatWithZoneName() {
}
@Test
- public void testDateFormatWithEscapedCharactersJdk8() {
- assumeJdk8();
- assertDate("01 December '18 12:00 AM; day in year: 335; week in year: 49",
- "dd MMMM ''yy hh:mm a; 'day in year': D; 'week in year': w",
- "UTC",
- null);
- }
-
- @Test
- public void testDateFormatWithEscapedCharactersJdk11OrNewer() {
- assumeJdk11OrNewer();
+ @Ignore("ambiguous results depending on the jdk version and implementation used")
Review comment:
That is interesting. Can you share the output (with concrete versions)? Is CLDR really so bad with backwards compatibility or was it just implemented incorrectly in Oracle JDK8?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] removed a comment on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884125527
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [87.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-883951385
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [80.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] kwin commented on a change in pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#discussion_r673040172
##########
File path: src/main/java/org/apache/sling/scripting/sightly/impl/engine/extension/FormatFilterExtension.java
##########
@@ -77,12 +81,14 @@ public Object call(final RenderContext renderContext, Object... arguments) {
if (hasPlaceHolders) {
return getFormattedString(runtimeObjectModel, source, formatObject);
}
+
try {
- // somebody will hate me for this
- new SimpleDateFormat(source);
+ // try to parse as DateTimeFormatter
Review comment:
The question is rather under which circumstances do you end in this area as there are 4 return statements before this line (which IMHO cover all configuration options...)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] Buuhuu merged pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
Buuhuu merged pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-sightly] sonarcloud[bot] commented on pull request #11: SLING-9989: migrate date formating to java.time API
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #11:
URL: https://github.com/apache/sling-org-apache-sling-scripting-sightly/pull/11#issuecomment-884059330
Kudos, SonarCloud Quality Gate passed! 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list) [85.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-sightly&pullRequest=11&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org