[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #144: JCRVLT-526 improved support of legacy timezone designators "+hhmm"

[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