Issue 299100043: DateTimePatternGenerator data loading - Java (Closed)
|
Created:
7 years, 11 months ago by sffc Modified:
7 years, 10 months ago CC:
markus.icu Base URL:
svn+icussh://source.icu-project.org/repos/icu/icu4j/trunk/ Visibility:
Public. |
DescriptionFirst change: Refactoring DateTimePatternGenerator data loading in preparation for data sink transformations.
Patch Set 1 #
Total comments: 7
Patch Set 2 : Implementing data sink logic. #
Total comments: 14
Patch Set 3 : Updating with Markus's first round of feedback. #
Total comments: 2
Patch Set 4 : Updating with Markus's second round of feedback. Also merging JUnit changes. #Patch Set 5 : Small refactoring changes. #MessagesTotal messages: 17
This is purely refactoring. I'm working on making the Java and C++ code layouts more similar, in preparation for adding data sinks. https://codereview.appspot.com/299100043/diff/1/main/tests/core/src/com/ibm/i... File main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java (right): https://codereview.appspot.com/299100043/diff/1/main/tests/core/src/com/ibm/i... main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java:810: dtpg.getCanonicalSkeletonAllowingDuplicates(cases[i])); This fixes a small bug I noticed from my previous commit.
Sign in to reply to this message.
https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:158: private String shortTimePattern; At a glance, most of your changes look reasonable. This added class instance field does not look good. Please change the code to avoid it. Maybe return it from the next method and then pass it into others.
Sign in to reply to this message.
I'll start working on the data sink stuff in this class soon. My first commit to this CR was just refactoring to update Java's code structure to match C++. https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:158: private String shortTimePattern; On 2016/05/23 00:49:07, markus.icu wrote: > At a glance, most of your changes look reasonable. > This added class instance field does not look good. Please change the code to > avoid it. Maybe return it from the next method and then pass it into others. C++ uses a field, and I wanted to make Java consistent. I can change both languages to pass down the string via return values instead.
Sign in to reply to this message.
https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:158: private String shortTimePattern; On 2016/05/23 06:19:28, sffc wrote: > On 2016/05/23 00:49:07, markus.icu wrote: > > At a glance, most of your changes look reasonable. > > This added class instance field does not look good. Please change the code to > > avoid it. Maybe return it from the next method and then pass it into others. > > C++ uses a field, and I wanted to make Java consistent. I can change both > languages to pass down the string via return values instead. Please do. This does not look like something we want to store permanently in the object.
Sign in to reply to this message.
Implementing data sink logic.
Sign in to reply to this message.
Refactoring DateTimePatternGenerator to use data sinks. https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:158: private String shortTimePattern; On 2016/05/27 22:49:59, markus.icu wrote: > On 2016/05/23 06:19:28, sffc wrote: > > On 2016/05/23 00:49:07, markus.icu wrote: > > > At a glance, most of your changes look reasonable. > > > This added class instance field does not look good. Please change the code > to > > > avoid it. Maybe return it from the next method and then pass it into others. > > > > C++ uses a field, and I wanted to make Java consistent. I can change both > > languages to pass down the string via return values instead. > > Please do. This does not look like something we want to store permanently in the > object. Done. https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:1712: appendItemNames[i] = "F" + i; I moved this initialization into a "fillInMissing" method run after consuming the CLDR data so that the data sink can detect and replace null values. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:36: import com.ibm.icu.impl.UResource.Value; Sorry, I'll fix the references. I think Eclipse adds them automatically when I click "add unimplemented methods" on a new Sink class. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:305: rb.getAllItemsWithFallback("calendar/" + calendarTypeToUse + "/availableFormats", sink); The data sink logic is much simpler for this case! https://codereview.appspot.com/299100043/diff/20001/main/tests/core/src/com/i... File main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java (right): https://codereview.appspot.com/299100043/diff/20001/main/tests/core/src/com/i... main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java:941: public void TestGetAppendItemFormat(){ I added a new test case, because CLDR data loading was not being tested. Is it ok to hard-code the expected results like I did here?
Sign in to reply to this message.
https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/1/main/classes/core/src/com/ibm... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:1712: appendItemNames[i] = "F" + i; On 2016/05/27 23:17:11, sffc wrote: > I moved this initialization into a "fillInMissing" method run after consuming > the CLDR data so that the data sink can detect and replace null values. This is good. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:36: import com.ibm.icu.impl.UResource.Value; On 2016/05/27 23:17:11, sffc wrote: > Sorry, I'll fix the references. I think Eclipse adds them automatically when I > click "add unimplemented methods" on a new Sink class. Yes, I have seen that too. I wonder if it would help to change the signatures of those base class methods to have the UResource. qualifier? https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:171: // combination patterns by hackTimes later in this method. "later in this method" -> "in later initialization steps" or similar? https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:194: private class AppendItemFormatsSink extends UResource.Sink { Please make all nested classes static if they do not need access to the outer class instance. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:227: } It looks like you only care about "dn" here. In that case, please "break" after this line so you don't spin your wheels for items after "dn". https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:282: AppendItemFormatsSink sink = new AppendItemFormatsSink(); Question (I don't know the bundle data structure): Would it make sense to combine multiple sinks into one by starting higher up in the tree and picking out the relevant sub-trees? For example, start at "calendar/" + calendarTypeToUse and pick out "appendItems" and "availableFormats"? (They will be seen in that order, since bundle tables are sorted.) https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:305: rb.getAllItemsWithFallback("calendar/" + calendarTypeToUse + "/availableFormats", sink); On 2016/05/27 23:17:11, sffc wrote: > The data sink logic is much simpler for this case! :-) Yes, the old code had to hack around in order to both enumerate and get parent-bundle fallbacks. https://codereview.appspot.com/299100043/diff/20001/main/tests/core/src/com/i... File main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java (right): https://codereview.appspot.com/299100043/diff/20001/main/tests/core/src/com/i... main/tests/core/src/com/ibm/icu/dev/test/format/DateTimeGeneratorTest.java:941: public void TestGetAppendItemFormat(){ On 2016/05/27 23:17:11, sffc wrote: > I added a new test case, because CLDR data loading was not being tested. Is it > ok to hard-code the expected results like I did here? It's ok, often we have to do this in order to do meaningful testing. We just have to adjust when we get new CLDR data. It might be worth adding a comment that the expected results will change with the locale data. We don't like everyone in Google code to do this because (a) they should test the results of their code, not our code and (b) because it's so painful to fix everyone else's tests before we can upgrade.
Sign in to reply to this message.
Updating with Markus's first round of feedback.
Sign in to reply to this message.
Updating with Markus's first round of feedback.
Sign in to reply to this message.
https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:171: // combination patterns by hackTimes later in this method. On 2016/05/28 18:49:25, markus.icu wrote: > "later in this method" -> "in later initialization steps" or similar? I moved the call to "hackTimes" into this method. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:194: private class AppendItemFormatsSink extends UResource.Sink { On 2016/05/28 18:49:25, markus.icu wrote: > Please make all nested classes static if they do not need access to the outer > class instance. I'll keep an eye out for places where I can make things static. In this case, the sink wants to access "getAppendItemFormat" and "setAppendItemFormat", which are non-static methods. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:227: } On 2016/05/28 18:49:25, markus.icu wrote: > It looks like you only care about "dn" here. In that case, please "break" after > this line so you don't spin your wheels for items after "dn". Done. https://codereview.appspot.com/299100043/diff/20001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:282: AppendItemFormatsSink sink = new AppendItemFormatsSink(); On 2016/05/28 18:49:25, markus.icu wrote: > Question (I don't know the bundle data structure): Would it make sense to > combine multiple sinks into one by starting higher up in the tree and picking > out the relevant sub-trees? For example, start at "calendar/" + > calendarTypeToUse and pick out "appendItems" and "availableFormats"? (They will > be seen in that order, since bundle tables are sorted.) Discussed. (appendItems and availableFormats are frequently aliased.)
Sign in to reply to this message.
LGTM Please adjust the comment if appropriate, and commit. https://codereview.appspot.com/299100043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:133: // we still need to make a dummy instance. It looks like the returnInfo is not a dummy, but temporary. Please adjust this comment.
Sign in to reply to this message.
Updating with Markus's second round of feedback. Also merging JUnit changes.
Sign in to reply to this message.
Committed as r38801. https://codereview.appspot.com/299100043/diff/60001/main/classes/core/src/com... File main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java (right): https://codereview.appspot.com/299100043/diff/60001/main/classes/core/src/com... main/classes/core/src/com/ibm/icu/text/DateTimePatternGenerator.java:133: // we still need to make a dummy instance. On 2016/06/06 23:08:28, markus.icu wrote: > It looks like the returnInfo is not a dummy, but temporary. Please adjust this > comment. Done.
Sign in to reply to this message.
Small refactoring changes.
Sign in to reply to this message.
Small changes to the Java version for C++ consistency.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
|
