[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
