Closed
Bug 1258789
Opened 8 years ago
Closed 8 years ago
Add WhitespaceAround checkstyle check & fix issues
Categories
(Firefox Build System :: Android Studio and Gradle Integration, defect)
Firefox Build System
Android Studio and Gradle Integration
All
Android
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mcomella, Assigned: mcomella)
References
(Blocks 1 open bug)
Details
Attachments
(17 files)
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
Grisha
:
review+
|
Details |
This comes up a lot with contributors – let's make review easier. This is whitespace in expressions like, "1+1" or "1+ 1", which should be "1 + 1".
Assignee | ||
Comment 1•8 years ago
|
||
See http://checkstyle.sourceforge.net/config_whitespace.html for all whitespace checks, e.g. we should also include WhitespaceAfter.
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45981/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45981/
Attachment #8740741 -
Flags: review?(gkruglov)
Attachment #8740742 -
Flags: review?(gkruglov)
Attachment #8740743 -
Flags: review?(gkruglov)
Attachment #8740744 -
Flags: review?(gkruglov)
Attachment #8740745 -
Flags: review?(gkruglov)
Attachment #8740746 -
Flags: review?(gkruglov)
Attachment #8740747 -
Flags: review?(gkruglov)
Attachment #8740748 -
Flags: review?(gkruglov)
Attachment #8740749 -
Flags: review?(gkruglov)
Attachment #8740750 -
Flags: review?(gkruglov)
Attachment #8740751 -
Flags: review?(gkruglov)
Attachment #8740752 -
Flags: review?(gkruglov)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45983/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45983/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45985/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45985/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45987/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45987/
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45989/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45989/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45991/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45991/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45993/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45993/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45995/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45995/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45997/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45997/
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45999/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45999/
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46001/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46001/
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46003/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46003/
Assignee | ||
Comment 14•8 years ago
|
||
Sorry in advance! :P This patch series is not yet complete – note that I intend to add all the default markers, which will remove the "tokens" property, which is why I don't care that they're not alphabetized.
Assignee: nobody → michael.l.comella
Comment 15•8 years ago
|
||
Comment on attachment 8740741 [details] MozReview Request: Bug 1258789 - Add WhitespaceAround for ASSIGN token. r=grisha https://reviewboard.mozilla.org/r/45981/#review42511 ::: mobile/android/app/checkstyle.xml:56 (Diff revision 1) > <module name="OuterTypeFilename"/> <!-- `class Lol` only in Lol.java --> > <module name="WhitespaceAfter"> > <!-- TODO: (bug 1263059) Remove specific tokens to enable CAST check. --> > <property name="tokens" value="COMMA, SEMI"/> > </module> > + <module name="WhitespaceAround"> Fun! I love stuff like this :)
Attachment #8740741 -
Flags: review?(gkruglov) → review+
Assignee | ||
Comment 16•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46149/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46149/
Attachment #8741057 -
Flags: review?(gkruglov)
Attachment #8741058 -
Flags: review?(gkruglov)
Attachment #8741059 -
Flags: review?(gkruglov)
Attachment #8741060 -
Flags: review?(gkruglov)
Attachment #8741061 -
Flags: review?(gkruglov)
Assignee | ||
Comment 17•8 years ago
|
||
It's an anti-pattern: https://blog.jooq.org/2014/12/08/dont-be-clever-the-double-curly-braces-anti-pattern/ Mobile-relevant summary: * It creates new types, bloating our APK * It can cause memory leaks by keeping a reference to the containing class Review commit: https://reviewboard.mozilla.org/r/46151/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46151/
Assignee | ||
Comment 18•8 years ago
|
||
Fix some cast issues while I was at it. Review commit: https://reviewboard.mozilla.org/r/46153/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46153/
Assignee | ||
Comment 19•8 years ago
|
||
Some issues remain. Review commit: https://reviewboard.mozilla.org/r/46155/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46155/
Assignee | ||
Comment 20•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46157/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46157/
Assignee | ||
Comment 21•8 years ago
|
||
That's the last of them! Please review quickly, not because it's important, but because it can cause painful bit-rot!
Comment 22•8 years ago
|
||
Comment on attachment 8741060 [details] MozReview Request: Bug 1258789 - Remove WhitespaceAround tokens filter and fix lcurly issues. r=grisha https://reviewboard.mozilla.org/r/46155/#review42637
Attachment #8741060 -
Flags: review?(gkruglov) → review+
Comment 23•8 years ago
|
||
Comment on attachment 8741061 [details] MozReview Request: Bug 1258789 - Remove allowEmptyLoops I added previously to simplify things. r=grisha https://reviewboard.mozilla.org/r/46157/#review42639
Attachment #8741061 -
Flags: review?(gkruglov) → review+
Comment 24•8 years ago
|
||
Comment on attachment 8741059 [details] MozReview Request: Bug 1258789 - Add div* token for WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/46153/#review42641
Attachment #8741059 -
Flags: review?(gkruglov) → review+
Updated•8 years ago
|
Attachment #8741058 -
Flags: review?(gkruglov) → review+
Comment 25•8 years ago
|
||
Comment on attachment 8741058 [details] MozReview Request: Bug 1258789 - Fix remaining rcurly issues by eliminating double curly bracket. r=grisha https://reviewboard.mozilla.org/r/46151/#review42647 lgtm
Updated•8 years ago
|
Attachment #8741057 -
Flags: review?(gkruglov) → review+
Comment 26•8 years ago
|
||
Comment on attachment 8741057 [details] MozReview Request: Bug 1258789 - Add rcurly and fix {} issues. r=grisha https://reviewboard.mozilla.org/r/46149/#review42649 i personally like {} more, but oh well.
Comment 27•8 years ago
|
||
Comment on attachment 8740752 [details] MozReview Request: Bug 1258789 - Add passing tokens to WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/46003/#review42651
Attachment #8740752 -
Flags: review?(gkruglov) → review+
Comment 28•8 years ago
|
||
Comment on attachment 8740751 [details] MozReview Request: Bug 1258789 - Add plus operator to WhitespaceAround and fix. r=grisha https://reviewboard.mozilla.org/r/46001/#review42655
Attachment #8740751 -
Flags: review?(gkruglov) → review+
Comment 29•8 years ago
|
||
Comment on attachment 8740750 [details] MozReview Request: Bug 1258789 - Add star to WhitespaceAround and fix. r=grisha https://reviewboard.mozilla.org/r/45999/#review42657
Attachment #8740750 -
Flags: review?(gkruglov) → review+
Updated•8 years ago
|
Attachment #8740749 -
Flags: review?(gkruglov) → review+
Comment 30•8 years ago
|
||
Comment on attachment 8740749 [details] MozReview Request: Bug 1258789 - Add shift operators to WhitespaceAround & fix. r=grisha https://reviewboard.mozilla.org/r/45997/#review42659
Updated•8 years ago
|
Attachment #8740748 -
Flags: review?(gkruglov) → review+
Comment 31•8 years ago
|
||
Comment on attachment 8740748 [details] MozReview Request: Bug 1258789 - Add a few more to WhitespaceAround and fix. r=grisha https://reviewboard.mozilla.org/r/45995/#review42661
Comment 32•8 years ago
|
||
Comment on attachment 8740747 [details] MozReview Request: Bug 1258789 - Add while to WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/45993/#review42663
Attachment #8740747 -
Flags: review?(gkruglov) → review+
Updated•8 years ago
|
Attachment #8740746 -
Flags: review?(gkruglov) → review+
Comment 33•8 years ago
|
||
Comment on attachment 8740746 [details] MozReview Request: Bug 1258789 - Add synchronized & non-failing try to WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/45991/#review42665
Comment 34•8 years ago
|
||
Comment on attachment 8740745 [details] MozReview Request: Bug 1258789 - Add switch for whitespaceAround. r=grisha https://reviewboard.mozilla.org/r/45989/#review42667
Attachment #8740745 -
Flags: review?(gkruglov) → review+
Comment 35•8 years ago
|
||
Comment on attachment 8740744 [details] MozReview Request: Bug 1258789 - Add if & return for WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/45987/#review42669
Attachment #8740744 -
Flags: review?(gkruglov) → review+
Comment 36•8 years ago
|
||
Comment on attachment 8740743 [details] MozReview Request: Bug 1258789 - Add literal catch & a few others that don't break for WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/45985/#review42671
Attachment #8740743 -
Flags: review?(gkruglov) → review+
Comment 37•8 years ago
|
||
Comment on attachment 8740742 [details] MozReview Request: Bug 1258789 - Add binary logic operators for WhitespaceAround. r=grisha https://reviewboard.mozilla.org/r/45983/#review42675
Attachment #8740742 -
Flags: review?(gkruglov) → review+
Comment 38•8 years ago
|
||
Next up, inconsistent indentation!
Assignee | ||
Comment 39•8 years ago
|
||
(In reply to :Grisha Kruglov from comment #26) > i personally like {} more, but oh well. I intend to send an email about this one. `{}` is valid in cases where you might want to use it (e.g. empty constructors for util classes) but not for places where you might not want to, e.g. `catch (...) {}` – it's ambiguous to catch, do nothing, and not comment about it. (In reply to :Grisha Kruglov from comment #38) > Next up, inconsistent indentation! Now that's not going to be fun to fix at. all.
Assignee | ||
Comment 40•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4aec5484727310c0ddba1d05142095391f570140 Bug 1258789 - Add WhitespaceAround for ASSIGN token. r=grisha https://hg.mozilla.org/integration/fx-team/rev/2581aa4a58e94998a75878242cbbd6cd4dfd2d0f Bug 1258789 - Add binary logic operators for WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/e035db2ceba60ba2601130239a9cf591e7381ee5 Bug 1258789 - Add literal catch & a few others that don't break for WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/53f8e479322be4b823b444f562b4dc06c4920b63 Bug 1258789 - Add if & return for WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/30c0b243df2dd2af133a5258c8bce100225285dc Bug 1258789 - Add switch for whitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/ebe249484944f34aab6da0539be018751c9a5444 Bug 1258789 - Add synchronized & non-failing try to WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/f1a36e0e657e0c0f1fbd0747e0b27dbc3c717a93 Bug 1258789 - Add while to WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/cdfaf8bc0e6c4fdd4138815020c8f371c11a5de8 Bug 1258789 - Add a few more to WhitespaceAround and fix. r=grisha https://hg.mozilla.org/integration/fx-team/rev/276a287d749033f33188a8b6973df7bfaecfa0d2 Bug 1258789 - Add shift operators to WhitespaceAround & fix. r=grisha https://hg.mozilla.org/integration/fx-team/rev/914c1cb7842b6d7f4370024c7629a6bb075e6c6f Bug 1258789 - Add star to WhitespaceAround and fix. r=grisha https://hg.mozilla.org/integration/fx-team/rev/a7e8cc02fc77d2356a4421ca0224e1c3982517a1 Bug 1258789 - Add plus operator to WhitespaceAround and fix. r=grisha https://hg.mozilla.org/integration/fx-team/rev/c0364af095efaa7d424931ffd0ddf04570f23a98 Bug 1258789 - Add passing tokens to WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/3281ebb862468d7d7b98ad9fbd439a24c799347a Bug 1258789 - Add rcurly and fix {} issues. r=grisha https://hg.mozilla.org/integration/fx-team/rev/7775ee20ad1c1778e040be3a759e2c27073e945c Bug 1258789 - Fix remaining rcurly issues by eliminating double curly bracket. r=grisha https://hg.mozilla.org/integration/fx-team/rev/f3d2612a4d276d2180f893773d4ea2a93dddfe59 Bug 1258789 - Add div* token for WhitespaceAround. r=grisha https://hg.mozilla.org/integration/fx-team/rev/6c7e7542fcd34b5c9c5faa7dd8c85256729961c4 Bug 1258789 - Remove WhitespaceAround tokens filter and fix lcurly issues. r=grisha https://hg.mozilla.org/integration/fx-team/rev/c07f4c92f8a60d99d1f2fa14704ce33ce347c4f8 Bug 1258789 - Remove allowEmptyLoops I added previously to simplify things. r=grisha
Comment 41•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4aec54847273 https://hg.mozilla.org/mozilla-central/rev/2581aa4a58e9 https://hg.mozilla.org/mozilla-central/rev/e035db2ceba6 https://hg.mozilla.org/mozilla-central/rev/53f8e479322b https://hg.mozilla.org/mozilla-central/rev/30c0b243df2d https://hg.mozilla.org/mozilla-central/rev/ebe249484944 https://hg.mozilla.org/mozilla-central/rev/f1a36e0e657e https://hg.mozilla.org/mozilla-central/rev/cdfaf8bc0e6c https://hg.mozilla.org/mozilla-central/rev/276a287d7490 https://hg.mozilla.org/mozilla-central/rev/914c1cb7842b https://hg.mozilla.org/mozilla-central/rev/a7e8cc02fc77 https://hg.mozilla.org/mozilla-central/rev/c0364af095ef https://hg.mozilla.org/mozilla-central/rev/3281ebb86246 https://hg.mozilla.org/mozilla-central/rev/7775ee20ad1c https://hg.mozilla.org/mozilla-central/rev/f3d2612a4d27 https://hg.mozilla.org/mozilla-central/rev/6c7e7542fcd3 https://hg.mozilla.org/mozilla-central/rev/c07f4c92f8a6
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•5 years ago
|
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 48 → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•