http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java File tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java (right):
http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java#newcode52 tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java:52: new String[]{"sun", "mon", "tue", "wed", "thu", "fri", "sat"}; On 2011/04/06 04:15:00, jat wrote:
The whitespace is easily fixed by changing the formatter:
whitespace / arrays / array initializers / before opening brace
Added that to this patch.
The lousy line breaks on assignments seems harder to fix -- the
options are no
breaks or break like the above. I think it is more likely that
assignments all
over the place will be made worse by this than the alternative, which
seems to
mostly hit field initializations.
Hey look at that, the brace formatting change killed 2 birds with one stone in this case. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java#newcode396 tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java:396: + "dayPeriodContext[@type=\"format\"]/" On 2011/04/05 13:43:27, jat wrote:
Why does this get indended again? If it is consistent I can live with
it, but
it seems wrong.
I think it looks wrong to us because we're used to not indenting before the string concatenation when formatted on a second line. The unwritten rule it seems to consistently follow is that new statements get one indent from the parent of the statement, (2 spaces), line continuation gets 2 indents from the parent statement (4 spaces). http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/GenerateGwtCldrData.java File tools/cldr-import/src/com/google/gwt/tools/cldr/GenerateGwtCldrData.java (right): http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/GenerateGwtCldrData.java#newcode59 tools/cldr-import/src/com/google/gwt/tools/cldr/GenerateGwtCldrData.java:59: UOption[] options = On 2011/04/05 13:43:27, jat wrote:
Wow, this one is really ugly: It splts the assignment in a weird
place, it puts
the opening brace on its own line yet puts the closing brace at the
end (with no
space even).
I definitely think this needs more tweaking.
The assignment break is consistent with the way assignments are formatted everywhere else (setting PROCESSORS above). I have no idea why it put the lead curly on its own line and spent about 30 minutes diddling unsuccessfully to make it go away. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java File tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java (right): http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java#newcode432 tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java:432: "type"); On 2011/04/05 13:43:27, jat wrote:
Why does it increase the indent level and then go back?
I can't find a specific setting that allows you to control this, but the general indentation rule is: 1 indent (2 spaces) is for a new statement 2 indents (4 spaces) is for a continuation of the previous line. In the olden days, I thought there used to be special cases for string concatenation, but I don't see them any more. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java File tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java (right): http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java#newcode100 tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java:100: .getVariantNotNull()); On 2011/04/05 13:43:27, jat wrote:
Another awkward split.
Consequence of the "builder pattern" formatter change we discussed back when that change was made. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java#newcode339 tools/cldr-import/src/com/google/gwt/tools/cldr/LocalizedNamesProcessor.java:339: .getRegions(locale.getLanguageNotNull() + "_" + locale.getScriptNotNull()); On 2011/04/05 13:43:27, jat wrote:
Yikes. Bad assignment split, and splitting at . rather than in the
argument
list looks really bad.
Again, the assignment split appears to me to be consistent throughout the reformatting. http://gwt-code-reviews.appspot.com/1402803/diff/6001/eclipse/external/cldr-tools/.classpath File eclipse/external/cldr-tools/.classpath (right): http://gwt-code-reviews.appspot.com/1402803/diff/6001/eclipse/external/cldr-tools/.classpath#newcode7 eclipse/external/cldr-tools/.classpath:7: <classpathentry kind="var" path="GWT_TOOLS/lib/icu4j/4.4.2/icu4j.jar" sourcepath="/GWT_TOOLS/lib/icu4j/4.4.2/icu4jsrc.jar"/> Not strictly related, but I couldn't import the project into eclipse without this change. http://gwt-code-reviews.appspot.com/1402803/diff/6001/eclipse/tools/cldr-import/.classpath File eclipse/tools/cldr-import/.classpath (right): http://gwt-code-reviews.appspot.com/1402803/diff/6001/eclipse/tools/cldr-import/.classpath#newcode12 eclipse/tools/cldr-import/.classpath:12: <classpathentry kind="var" path="GWT_TOOLS/lib/cldr/1.8.1/cldr.jar" /> Not strictly related, but lots of unresolved classes without this. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
