Add WhitespaceAround checkstyle check & fix issues

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
3 days ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(17 attachments)

58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
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".
See http://checkstyle.sourceforge.net/config_whitespace.html for all whitespace checks, e.g. we should also include WhitespaceAfter.
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)
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 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+
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/
That's the last of them! Please review quickly, not because it's important, but because it can cause painful bit-rot!
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 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 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+
Attachment #8741058 - Flags: review?(gkruglov) → review+
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
Attachment #8741057 - Flags: review?(gkruglov) → review+
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 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 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 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+
Attachment #8740749 - Flags: review?(gkruglov) → review+
Comment on attachment 8740749 [details]
MozReview Request: Bug 1258789 - Add shift operators to WhitespaceAround & fix. r=grisha

https://reviewboard.mozilla.org/r/45997/#review42659
Attachment #8740748 - Flags: review?(gkruglov) → review+
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 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+
Attachment #8740746 - Flags: review?(gkruglov) → review+
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 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 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 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 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+
Next up, inconsistent indentation!
(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.
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
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.