You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2021/06/07 15:40:48 UTC
[GitHub] [jackrabbit-filevault] kwin opened a new pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
kwin opened a new pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144
Based on a suggestion by Julian Sedding
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
stoerr commented on a change in pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144#discussion_r646872076
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -224,21 +222,17 @@ public boolean requiresRestart() {
public Calendar getDateProperty(String name) {
Calendar result = null;
try {
- // TODO: add timezone if not there?
String p = getProperty(name);
if (p != null) {
- result = ISO8601.parse(p);
- if (result == null) {
- // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
- // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
- Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
- if (splittedDate.matches()) {
- String tzminutes = splittedDate.group("tzMinutes");
- tzminutes = tzminutes != null ? tzminutes : "00";
- String reformattedDate = splittedDate.group("dateUptoTzHours") + ":" + tzminutes;
- result = ISO8601.parse(reformattedDate);
- }
+ ZonedDateTime zonedDateTime;
+ try {
+ // supports date in format new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSXXX")
+ zonedDateTime = ZonedDateTime.parse(p, DateTimeFormatter.ISO_OFFSET_DATE_TIME);
+ } catch (DateTimeParseException e) {
+ // support dates given out via new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ"); as well (used in package-maven-plugin till version 1.0.3, compare with https://issues.apache.org/jira/browse/JCRVLT-276)
+ zonedDateTime = ZonedDateTime.parse(p, DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSX"));
Review comment:
That undoubtedly looks much cleaner than the pattern replacement approach. So, if that is backwards compatible to ISO8601 and does parse the +0200 timezone format variant, then that's completely fine with me. Mind you - the test I wrote did not intend to see whether it still parses all date formatting variants ISO8601.parse understands, if there are some, since it was still using ISO8601.parse so far. So you might want to check whether you need to extend the test somewhat.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] sonarcloud[bot] commented on pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144#issuecomment-856211259
SonarCloud Quality Gate failed.
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/B.png' alt='B' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_jackrabbit-filevault&pullRequest=144&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100.png' alt='100.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_jackrabbit-filevault&pullRequest=144&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_jackrabbit-filevault&pullRequest=144&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_jackrabbit-filevault&pullRequest=144&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_jackrabbit-filevault&pullRequest=144&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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
stoerr commented on a change in pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144#discussion_r646872076
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -224,21 +222,17 @@ public boolean requiresRestart() {
public Calendar getDateProperty(String name) {
Calendar result = null;
try {
- // TODO: add timezone if not there?
String p = getProperty(name);
if (p != null) {
- result = ISO8601.parse(p);
- if (result == null) {
- // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
- // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
- Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
- if (splittedDate.matches()) {
- String tzminutes = splittedDate.group("tzMinutes");
- tzminutes = tzminutes != null ? tzminutes : "00";
- String reformattedDate = splittedDate.group("dateUptoTzHours") + ":" + tzminutes;
- result = ISO8601.parse(reformattedDate);
- }
+ ZonedDateTime zonedDateTime;
+ try {
+ // supports date in format new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSXXX")
+ zonedDateTime = ZonedDateTime.parse(p, DateTimeFormatter.ISO_OFFSET_DATE_TIME);
+ } catch (DateTimeParseException e) {
+ // support dates given out via new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ"); as well (used in package-maven-plugin till version 1.0.3, compare with https://issues.apache.org/jira/browse/JCRVLT-276)
+ zonedDateTime = ZonedDateTime.parse(p, DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSX"));
Review comment:
That undoubtedly looks much cleaner than the pattern replacement approach. So, if that is backwards compatible to ISO8601 and does parse the +0200 timezone format variant, then that's completely fine with me. Mind you - the test I wrote did not intend to see whether it still parses all date formatting variants ISO8601.parse understands, if there are some, since it was still using ISO8601.parse so far. So you might want to check whether you need to extend the test somewhat.
BTW: I'd set the DateTimeFormatter.ofPattern() into constants, as according to the specs a DateTimeFormatter is immutable and is thread-safe.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin merged pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
kwin merged pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144#discussion_r647363593
##########
File path: vault-core/src/test/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImplTest.java
##########
@@ -93,9 +94,6 @@ public void testDateFormat() {
checkDateParsing("2021-05-26T15:12:21.673+0200","2021-05-26T13:12:21.673Z");
checkDateParsing("2021-05-26T15:12:21.673-0230","2021-05-26T17:42:21.673Z");
- // missing timezone is treated as UTC. TODO is that right?
- checkDateParsing("2021-05-26T15:12:21.673","2021-05-26T15:12:21.673Z");
-
Review comment:
That date format was never produced and therefore is not supported (i.e. returns null)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
stoerr commented on a change in pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144#discussion_r647747268
##########
File path: vault-core/src/test/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImplTest.java
##########
@@ -93,9 +94,6 @@ public void testDateFormat() {
checkDateParsing("2021-05-26T15:12:21.673+0200","2021-05-26T13:12:21.673Z");
checkDateParsing("2021-05-26T15:12:21.673-0230","2021-05-26T17:42:21.673Z");
- // missing timezone is treated as UTC. TODO is that right?
- checkDateParsing("2021-05-26T15:12:21.673","2021-05-26T15:12:21.673Z");
-
Review comment:
It didn't return null, as the test says. But never mind.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"
Posted by GitBox <gi...@apache.org>.
stoerr commented on a change in pull request #144:
URL: https://github.com/apache/jackrabbit-filevault/pull/144#discussion_r646874602
##########
File path: vault-core/src/test/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImplTest.java
##########
@@ -93,9 +94,6 @@ public void testDateFormat() {
checkDateParsing("2021-05-26T15:12:21.673+0200","2021-05-26T13:12:21.673Z");
checkDateParsing("2021-05-26T15:12:21.673-0230","2021-05-26T17:42:21.673Z");
- // missing timezone is treated as UTC. TODO is that right?
- checkDateParsing("2021-05-26T15:12:21.673","2021-05-26T15:12:21.673Z");
-
Review comment:
What happens now if the timezone is missing? I don't know whether anything produces that date format, but at least I'm a fan of specifying such edge cases in the test, even if the documentation doesn't really say anything about that. Not sure whether it's a problem if that behavior changes. That throws an exception now, right?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org