[java] Figure out how to make clang-format and presubmit harmonize |
||||||
Issue descriptionCurrently, clang-format and presubmit both do some java formatting checking, and sometimes they disagree (see blockers). On "[chromium-dev] Proposal: turning on clang format for all the repo", I said: """Having both on is silly, so I don't know if there's much point in making them match. If y'all don't consider clang-format good enough to be used for java code and prefer manually formatting your java code to match the presubmit thingy, we should disable clang-format. (And fix the issues that are considered blockers -- let me know which bugs these are.) Else we should disable the presubmit tool. So the bar shouldn't be "do they match" but "is clang-format good enough".""" I don't write any Java code, so I didn't feel comfortable making this call. On the other hand, that thread was 3 months ago and we still haven't resolved this, so maybe nobody else feels empowered to make this call either. +styleguide/java/OWNERS, do you have an opinion here? If clang-format is at all close enough to being good enough, I feel we should disable the presubmit, since clang-format can't just complain but also fix.
,
Sep 26 2017
Just played around a bit with .java "git cl format", and couldn't come up with any place where it got it wrong. I'll go ahead and remove checkstyle.
,
Sep 26 2017
I agree that I think clang-format is good enough at this point. We can follow up with any new issues with the normal process. It'd be nice to have clang-format running now instead of checkstyle.
,
Sep 26 2017
Note: hadn't looked at the blocking bugs before I commented. I still think it's "good enough" to just use without checkstyle, but I think we'll want to follow-up and try to fix bug 740162 and bug 728623 upstream (or at least file clang-format bugs for them)
,
Sep 26 2017
Also - clang-format is missing import ordering. It even support sorting imports for javascript, but there doesn't seem to be support for sorting import statements.
,
Sep 26 2017
Can you file a blocker for that? Given that clang-format can sort #includes, implementing that hopefully won't be too hard. (I can give it a shot, if you give me a good description on what it should do)
,
Sep 26 2017
,
Sep 26 2017
Filed. :)
,
Sep 28 2017
,
Sep 28 2017
Also realized it's missing unused-import detection (filed as a blocker as well). Rather than removing checkstyle, I think we should instead just disable all but its import checks. If clang-format learns to do these other things, then we can fully remove it.
,
Oct 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/82795d9734f7a20188e55987ec3d5e7952be16b5 commit 82795d9734f7a20188e55987ec3d5e7952be16b5 Author: Andrew Grieve <agrieve@chromium.org> Date: Mon Oct 02 19:50:46 2017 Android: Remove checkstyle checks that overlap with clang-format Bug: 768576 Change-Id: Iaec5ac6a8520c45d4d71e6f34f8d05f5d24b5106 Reviewed-on: https://chromium-review.googlesource.com/693516 Reviewed-by: Nico Weber <thakis@chromium.org> Reviewed-by: Tommy Nyquist <nyquist@chromium.org> Commit-Queue: Andrew Grieve <agrieve@chromium.org> Cr-Commit-Position: refs/heads/master@{#505738} [modify] https://crrev.com/82795d9734f7a20188e55987ec3d5e7952be16b5/tools/android/checkstyle/chromium-style-5.0.xml
,
Oct 3 2017
Should be fixed with the change to disable checkstyle formatting checks.
,
Nov 27
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by thakis@chromium.org
, Sep 25 2017