Issue 295670043: Update DateIntervalInfo to the newer version of ResourceSink (Java) (Closed)
DescriptionUpdate DateIntervalInfo to the newer version of ResourceSink (Java)
Patch Set 1 #
Total comments: 18
Patch Set 2 : Update DateIntervalInfo to new version of ResourceSink in (Java) #MessagesTotal messages: 6
https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:419: private static final class DateIntervalSink extends UResource.Sink { The change is small. I only moved stuff around. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:533: private CharSequence validateAndProcessPatternLetter(CharSequence patternLetter) { This is exactly the same method as before. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:557: private void setIntervalPatternIfAbsent(String currentSkeleton, String lrgDiffCalUnit, Value intervalPattern) { This is exactly the same method as before, except that currentSkeleton became a parameter instead of a field.
Sign in to reply to this message.
Looks good; just a couple suggestions. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:419: private static final class DateIntervalSink extends UResource.Sink { On 2016/06/30 04:12:01, fabalbon wrote: > The change is small. I only moved stuff around. Acknowledged. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:451: continue; Consider breaking after INTERVAL_FORMATS_KEY is encountered so you don't spin your wheels looking at the other keys once you've found it. (It's a tip Markus gave me on one of my code reviews a few weeks ago.) If INTERVAL_FORMATS_KEY is the only one you need, have you considered putting that path directly into the getAllItemsWithFallback? That method might not work though since the value could be an alias. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:455: if (value.getType() == ICUResourceBundle.ALIAS && key.contentEquals(INTERVAL_FORMATS_KEY)) { key.contentEquals(INTERVAL_FORMATS_KEY) will always be true here. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:468: } Optional: ... else { // Encountered unexpected data; silently ignore. } https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:493: } Optional: ... else { // Encountered unexpected data; silently ignore. } https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:494: } Optional: ... else { // Encountered unexpected data; silently ignore. } https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:533: private CharSequence validateAndProcessPatternLetter(CharSequence patternLetter) { On 2016/06/30 04:12:01, fabalbon wrote: > This is exactly the same method as before. Acknowledged. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:557: private void setIntervalPatternIfAbsent(String currentSkeleton, String lrgDiffCalUnit, Value intervalPattern) { On 2016/06/30 04:12:01, fabalbon wrote: > This is exactly the same method as before, except that currentSkeleton became a > parameter instead of a field. Acknowledged.
Sign in to reply to this message.
https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:451: continue; On 2016/06/30 05:34:46, sffc wrote: > Consider breaking after INTERVAL_FORMATS_KEY is encountered so you don't spin > your wheels looking at the other keys once you've found it. (It's a tip Markus > gave me on one of my code reviews a few weeks ago.) > > If INTERVAL_FORMATS_KEY is the only one you need, have you considered putting > that path directly into the getAllItemsWithFallback? That method might not work > though since the value could be an alias. On second thought, if the INTERVAL_FORMATS_KEY tables contain different data going up through the parent bundles, breaking after you find one of those keys might cause you to not see all of the available data. You can try adding the break and seeing if any test cases fail. That being said, looking at the code in public void setup(ULocale locale), it seems to me that the data sink is going to be called multiple times to handle alias fallbacks. Why not handle the aliases here automatically by passing the full path to getAllItemsWithFallback?
Sign in to reply to this message.
https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:451: continue; On 2016/06/30 06:41:47, sffc wrote: > On 2016/06/30 05:34:46, sffc wrote: > > Consider breaking after INTERVAL_FORMATS_KEY is encountered so you don't spin > > your wheels looking at the other keys once you've found it. (It's a tip > Markus > > gave me on one of my code reviews a few weeks ago.) > > > > If INTERVAL_FORMATS_KEY is the only one you need, have you considered putting > > that path directly into the getAllItemsWithFallback? That method might not > work > > though since the value could be an alias. > > On second thought, if the INTERVAL_FORMATS_KEY tables contain different data > going up through the parent bundles, breaking after you find one of those keys > might cause you to not see all of the available data. You can try adding the > break and seeing if any test cases fail. > > That being said, looking at the code in public void setup(ULocale locale), it > seems to me that the data sink is going to be called multiple times to handle > alias fallbacks. Why not handle the aliases here automatically by passing the > full path to getAllItemsWithFallback? The reason why I'm loading the calendar_type (e.g. gregorian) table and not the intervalFormats table directly is because in root intervalFormats can be an alias to another calendar type (e.g. chinese/intervalFormats -> gregorian/intervalFormats). The data I have to load is the union of all the elements in intervalFormats for the requested locale, it's fallbacks and all its aliases. If I request intervalFormats directly, I'll get the requested locale and its fallbacks, but I won't have access to it's alias thus I won't be able to load the its data. That's the reason why I need to load the containing table. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:455: if (value.getType() == ICUResourceBundle.ALIAS && key.contentEquals(INTERVAL_FORMATS_KEY)) { On 2016/06/30 05:34:46, sffc wrote: > key.contentEquals(INTERVAL_FORMATS_KEY) will always be true here. Thanks! I missed this when merging the two old methods into this one. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:468: } On 2016/06/30 05:34:45, sffc wrote: > Optional: > > ... else { > // Encountered unexpected data; silently ignore. > } Acknowledged. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:493: } On 2016/06/30 05:34:46, sffc wrote: > Optional: > > ... else { > // Encountered unexpected data; silently ignore. > } Acknowledged. https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:494: } On 2016/06/30 05:34:45, sffc wrote: > Optional: > > ... else { > // Encountered unexpected data; silently ignore. > } Acknowledged.
Sign in to reply to this message.
Update DateIntervalInfo to new version of ResourceSink in (Java)
Sign in to reply to this message.
LGTM https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java (right): https://codereview.appspot.com/295670043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateIntervalInfo.java:451: continue; On 2016/06/30 18:38:47, fabalbon wrote: > On 2016/06/30 06:41:47, sffc wrote: > > On 2016/06/30 05:34:46, sffc wrote: > > > Consider breaking after INTERVAL_FORMATS_KEY is encountered so you don't > spin > > > your wheels looking at the other keys once you've found it. (It's a tip > > Markus > > > gave me on one of my code reviews a few weeks ago.) > > > > > > If INTERVAL_FORMATS_KEY is the only one you need, have you considered > putting > > > that path directly into the getAllItemsWithFallback? That method might not > > work > > > though since the value could be an alias. > > > > On second thought, if the INTERVAL_FORMATS_KEY tables contain different data > > going up through the parent bundles, breaking after you find one of those keys > > might cause you to not see all of the available data. You can try adding the > > break and seeing if any test cases fail. > > > > That being said, looking at the code in public void setup(ULocale locale), it > > seems to me that the data sink is going to be called multiple times to handle > > alias fallbacks. Why not handle the aliases here automatically by passing the > > full path to getAllItemsWithFallback? > > The reason why I'm loading the calendar_type (e.g. gregorian) table and not the > intervalFormats table directly is because in root intervalFormats can be an > alias to another calendar type (e.g. chinese/intervalFormats -> > gregorian/intervalFormats). > > The data I have to load is the union of all the elements in intervalFormats for > the requested locale, it's fallbacks and all its aliases. If I request > intervalFormats directly, I'll get the requested locale and its fallbacks, but I > won't have access to it's alias thus I won't be able to load the its data. > That's the reason why I need to load the containing table. Acknowledged.
Sign in to reply to this message.
|
