LGTM http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt File java/release_notes.txt (right): http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt#newcode3 java/release_notes.txt:3: - AR, BA, BF, CR, DE, EC, ES, KZ, MK, NC, NG, PF, SB, UZ, 001 001 -> non-geo entity 882 (I think we fixed an example number).
Thanks for the change, David! Since most of my comments are about things that should change internally as well, maybe it is easier to make an internal CL first about those changes. Cheers, Shaopeng http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt File java/release_notes.txt (right): http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt#newcode4 java/release_notes.txt:4: - Geocoding data updates for country calling codes 54 (AR) and 81 (JP), new data for 234 (NG) seems we are following 80-char rule in this file http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xml File resources/PhoneNumberMetaData.xml (right): http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xm... resources/PhoneNumberMetaData.xml:6502: 6[12]| 6[12] should be combined with 9[12]. Also needs to fix internal. http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xm... resources/PhoneNumberMetaData.xml:15658: 2[2-6]| 2[2-6] can be combined with 3[2-6] http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xm... resources/PhoneNumberMetaData.xml:17480: 6[129]| 6[129] can be combined with 8[129] http://codereview.appspot.com/6349078/diff/1/resources/geocoding/en/81.txt File resources/geocoding/en/81.txt (right): http://codereview.appspot.com/6349078/diff/1/resources/geocoding/en/81.txt#ne... resources/geocoding/en/81.txt:16: # ja/81.txt and translated with Freebase. we should mention there are manual edit after translating with Freebase. This should also be done in the internal file... http://codereview.appspot.com/6349078/diff/1/resources/geocoding/es/54.txt File resources/geocoding/es/54.txt (right): http://codereview.appspot.com/6349078/diff/1/resources/geocoding/es/54.txt#ne... resources/geocoding/es/54.txt:16: # http://www.cnc.gov.ar/infotecnica/numeracion/indicativosinter.asp (2011-07-11) this date should probably be updated. We need to do this in the internal code as well. Sorry we didn't notice it in the internal review.
http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt File java/release_notes.txt (right): http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt#newcode3 java/release_notes.txt:3: - AR, BA, BF, CR, DE, EC, ES, KZ, MK, NC, NG, PF, SB, UZ, 001 On 2012/07/06 10:07:50, lararennie wrote: > 001 -> non-geo entity 882 > > (I think we fixed an example number). Done. http://codereview.appspot.com/6349078/diff/1/java/release_notes.txt#newcode4 java/release_notes.txt:4: - Geocoding data updates for country calling codes 54 (AR) and 81 (JP), new data for 234 (NG) On 2012/07/06 10:32:51, Shaopeng wrote: > seems we are following 80-char rule in this file This rule seems to be violated quite often below. It looks like 100 was the rule prior to April 24th, was this changed to 80 afterwards? http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xml File resources/PhoneNumberMetaData.xml (right): http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xm... resources/PhoneNumberMetaData.xml:15658: 2[2-6]| On 2012/07/06 10:32:51, Shaopeng wrote: > 2[2-6] can be combined with 3[2-6] Done. http://codereview.appspot.com/6349078/diff/1/resources/PhoneNumberMetaData.xm... resources/PhoneNumberMetaData.xml:17480: 6[129]| On 2012/07/06 10:32:51, Shaopeng wrote: > 6[129] can be combined with 8[129] Done.
http://codereview.appspot.com/6349078/diff/1/resources/geocoding/en/81.txt File resources/geocoding/en/81.txt (right): http://codereview.appspot.com/6349078/diff/1/resources/geocoding/en/81.txt#ne... resources/geocoding/en/81.txt:16: # ja/81.txt and translated with Freebase. On 2012/07/06 10:32:51, Shaopeng wrote: > we should mention there are manual edit after translating with Freebase. This > should also be done in the internal file... Done. http://codereview.appspot.com/6349078/diff/1/resources/geocoding/es/54.txt File resources/geocoding/es/54.txt (right): http://codereview.appspot.com/6349078/diff/1/resources/geocoding/es/54.txt#ne... resources/geocoding/es/54.txt:16: # http://www.cnc.gov.ar/infotecnica/numeracion/indicativosinter.asp (2011-07-11) On 2012/07/06 10:32:51, Shaopeng wrote: > this date should probably be updated. We need to do this in the internal code as > well. Sorry we didn't notice it in the internal review. Should the date be updated, or is it sufficient to say the file was manually edited after generation?
On 2012/07/06 12:52:53, davinci_google wrote: the Js metadata needs to be regenerated, otherwise LGTM
