[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
ping http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
LGETM I still don't like some of the changes that are being made by these settings (in particular assignments wrap very strangely), but I guess it is as good as we are going to get at the moment. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
On 2011/04/15 15:35:21, zundel wrote: ping Despite my minor complaints, but this matches what was proposed so LGTM. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
LGTM http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
I merged up, so rietveld thinks that every file changed, but in fact, there are no changes other than the removal of several files not related to the autoformat. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
I still the the assignment line breaks are really ugly this way, but if others would rather avoid long lines no matter how ugly the result is, I can go with it. http://gwt-code-reviews.appspot.com/1402803/diff/6001/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/6001/tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java#newcode51 tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java:51: private static final String[] DAYS = new String[] { Thanks. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
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
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
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#newcode396 tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java:396: + dayPeriodContext[@type=\format\]/ On 2011/04/06 15:33:49, zundel wrote: 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). I disagree, but I won't fight it (mostly because it appears to be a change in the Eclipse formatter that I didn't find a knob to control). By that logic, every succeeding line with + should be indented further, but it isn't (and would be very ugly if it did). It also represents a change to the formatting we have been using for 4+ years. I think there is an argument for indenting again when there is another level of nesting, like: foo(long,list,of,arguments, bar(more,arguments,here, and,here)) but that isn't what is happening here. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
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#newcode396 tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java:396: + dayPeriodContext[@type=\format\]/ On 2011/04/06 15:39:55, jat wrote: On 2011/04/06 15:33:49, zundel wrote: 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). I disagree, but I won't fight it (mostly because it appears to be a change in the Eclipse formatter that I didn't find a knob to control). By that logic, every succeeding line with + should be indented further, but it isn't (and would be very ugly if it did). It also represents a change to the formatting we have been using for 4+ years. I think there is an argument for indenting again when there is another level of nesting, like: foo(long,list,of,arguments, bar(more,arguments,here, and,here)) but that isn't what is happening here. The rule I think its following is illustrated below at 399+ line 399 is the start of a new line line 400 is the continuation of line 399 - it gets 2 indents line 401 is also a continutation of 399 so it also gets 2 indents... from tthe start of 399. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
cldr-import mostly looks good, but there are a few consistent issues. 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}; Why is it dropping the space before {? Also, the line break seems odd, though I could live with it. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java#newcode320 tools/cldr-import/src/com/google/gwt/tools/cldr/DateTimeFormatInfoProcessor.java:320: fallbackCategory == null ? Collections.String, String emptyMap() : localeData.getEntries( Also here the line break seems weird. 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\]/ Why does this get indended again? If it is consistent I can live with it, but it seems wrong. 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 = 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. 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); Why does it increase the indent level and then go back? http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java#newcode633 tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java:633: .fromString(localeName); Yuck - I greatly prefer splitting method calls only when necessary to make it fit, rather than just so it can fit one more work on the previous line. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java#newcode653 tools/cldr-import/src/com/google/gwt/tools/cldr/LocaleData.java:653: * specified category. Previously, the autoformatter was set to indent 4 characters for wrapped Javadoc, which was the original text. Now it seems like it is back to the default of lining it up with the first line. 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()); Another awkward split. 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()); Yikes. Bad assignment split, and splitting at . rather than in the argument list looks really bad. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/Processor.java File tools/cldr-import/src/com/google/gwt/tools/cldr/Processor.java (right): http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/src/com/google/gwt/tools/cldr/Processor.java#newcode142 tools/cldr-import/src/com/google/gwt/tools/cldr/Processor.java:142: new BufferedWriter(new OutputStreamWriter(new FileOutputStream(f), UTF-8)), false); Another really bad split. http://gwt-code-reviews.appspot.com/1402803/diff/1/tools/cldr-import/test/com/google/gwt/tools/cldr/LocaleDataTest.java File tools/cldr-import/test/com/google/gwt/tools/cldr/LocaleDataTest.java (right):
[gwt-contrib] Re: Auto-formats the GWT tools projects (excluding api-checker covered in (issue1402803)
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}; The whitespace is easily fixed by changing the formatter: whitespace / arrays / array initializers / before opening brace 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. http://gwt-code-reviews.appspot.com/1402803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors