Issue 309100043: ICU ticket #12666: Revise data loading sink, remove CalendarData reference (Closed)
DescriptionICU ticket #12666: Revise data loading sink, remove CalendarData reference
Patch Set 1 #
Total comments: 12
Patch Set 2 : Minor updates, responding to review. #Patch Set 3 : Update calendar loading with fallback to Gregorian. Also check for ARRAY of patterns. #
Total comments: 2
Patch Set 4 : Revise comparison with string constant #MessagesTotal messages: 12
Ready for your first review.
Sign in to reply to this message.
https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java (right): https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:910: private static enum DateTimeUnit { Optional: Inner enums are implicitly static. It's not necessary to add the static keyword. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1081: if (unit.relUnit == null) { // Should this be outside the loop? Having this check inside the loop with a 'continue' is the behavior equivalent to the previous sink's behavior. It will all depend on if the value of unit.relUnit changes while iterating over this table. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1126: // Handle Display Name for PLAIN direction for some units. Check the indentation on this method. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1150: if (key.contentEquals("dn")) { What would happen here if the type is not ICUResourceBundle.STRING? Wouldn't it be better to check (value.getType() == ICUResourceBundle.STRING && key.contentEquals("dn")) ? https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1225: calType = "gergorian"; typo: gregorian https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1228: ICUResourceBundle patternsRb = r.findWithFallback(resourcePath); Double check the implementation of CalendarData#get. It uses gregorian when 'calType' is not found.
Sign in to reply to this message.
https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java (right): https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:910: private static enum DateTimeUnit { On 2016/08/22 18:55:21, fabalbon wrote: > Optional: Inner enums are implicitly static. It's not necessary to add the > static keyword. Done. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1081: if (unit.relUnit == null) { // Should this be outside the loop? On 2016/08/22 18:55:22, fabalbon wrote: > Having this check inside the loop with a 'continue' is the behavior equivalent > to the previous sink's behavior. It will all depend on if the value of > unit.relUnit changes while iterating over this table. The value cannot change while iterating. I'll remove the comment. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1126: // Handle Display Name for PLAIN direction for some units. On 2016/08/22 18:55:22, fabalbon wrote: > Check the indentation on this method. I'm removing comment and updating indentation. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1150: if (key.contentEquals("dn")) { On 2016/08/22 18:55:22, fabalbon wrote: > What would happen here if the type is not ICUResourceBundle.STRING? Wouldn't it > be better to check (value.getType() == ICUResourceBundle.STRING && > key.contentEquals("dn")) ? Done. https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1228: ICUResourceBundle patternsRb = r.findWithFallback(resourcePath); On 2016/08/22 18:55:21, fabalbon wrote: > Double check the implementation of CalendarData#get. It uses gregorian when > 'calType' is not found. That's the intent of the calType selection above, from the calendar default. If the gregorian DateTimePatterns are not found for the locale, then it will fall back, eventually to root. Note that the type of patternsRb always seems to be ARRAY.
Sign in to reply to this message.
https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java (right): https://codereview.appspot.com/309100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1228: ICUResourceBundle patternsRb = r.findWithFallback(resourcePath); On 2016/08/24 00:25:53, CraigC wrote: > On 2016/08/22 18:55:21, fabalbon wrote: > > Double check the implementation of CalendarData#get. It uses gregorian when > > 'calType' is not found. > > That's the intent of the calType selection above, from the calendar default. If > the gregorian DateTimePatterns are not found for the locale, then it will fall > back, eventually to root. > > Note that the type of patternsRb always seems to be ARRAY. Here you are getting the name of the default calendar for the current locale, and then you requesting that calendar's data. But what I mean is that CalendarData has a hardcoded fallback to gregorian in case the requested calendar's data is no found. It might be that the resource bundle will always have data for the current locale's default calendar so this hardcoded fallback might not be necessary, but we should probably double check this.
Sign in to reply to this message.
https://codereview.appspot.com/309100043/diff/40001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java (right): https://codereview.appspot.com/309100043/diff/40001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1229: if (patternsRb == null && calType != "gregorian") { In java you should use !calType.equals("gregorian")
Sign in to reply to this message.
https://codereview.appspot.com/309100043/diff/40001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java (right): https://codereview.appspot.com/309100043/diff/40001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/RelativeDateTimeFormatter.java:1229: if (patternsRb == null && calType != "gregorian") { On 2016/08/26 17:58:19, fabalbon wrote: > In java you should use !calType.equals("gregorian") Done.
Sign in to reply to this message.
Should you close this issue? You sent your latest changes on https://codereview.appspot.com/302600043/
Sign in to reply to this message.
On 2016/08/27 00:59:40, fabalbon wrote: > Should you close this issue? You sent your latest changes on > https://codereview.appspot.com/302600043/ Sorry, I got confused! The two names are very similar haha
Sign in to reply to this message.
|
