Replace checkstyle with Spotless & Google Java Style#624
Replace checkstyle with Spotless & Google Java Style#624rymurr merged 3 commits intoprojectnessie:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #624 +/- ##
============================================
+ Coverage 69.34% 72.68% +3.33%
- Complexity 142 143 +1
============================================
Files 254 254
Lines 9807 10814 +1007
Branches 898 911 +13
============================================
+ Hits 6801 7860 +1059
+ Misses 2567 2484 -83
- Partials 439 470 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I'm not sure I'm a big fan of Google style (especially the 100 characters limit): it seems to force every statement to be wrapped. Are they other alternatives with good support for IDE (I checked the Eclipse plugin for google formatter, which seems to install easily although manually. I've no idea if it also works with version of the google plugin. |
1dbd729 to
46fa2c5
Compare
|
If everyone fine with this PR, we should probably merge it manually to keep the separate commits for 1. the text file + pom changes and 2. the "automatic spotless:apply". |
nastra
left a comment
There was a problem hiding this comment.
LGTM (I only reviewed the first commit). I agree it would make sense to merge those 2 commits without squashing them
|
lgtm |
|
I noticed this morning that the plugin complained about some files found under the |
Huh? Haven't seen it complain about anything in |
|
While trying spotless more, I found a couple of issues:
Maybe we should keep checkstyle for now? The lack of skip is kind of annoying to me (sometimes while I'm developing, I have temporary files which like logs which are often flagged by tools, but usually I use skip until I'm ready to clean up things before pushing my changes). |
laurentgo
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 616 files reviewed, 2 unresolved discussions (waiting on @laurentgo and @snazy)
pom.xml, line 994 at r2 (raw file):
<format> <includes> <include>**/*.xml</include>
This causes all the XML files across all modules to be checked once by the top level module and then latter by the owning module. Tbh, it feels too broad, and maybe we should only check things under ${project.basedir}/src
Moved the spotless-plugin into a new profile, which can be deactivated with Not sure whether the name-style checks are that much of a problem. It's something that can be spotted in reviews. |
snazy
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 616 files reviewed, 2 unresolved discussions (waiting on @laurentgo)
pom.xml, line 994 at r2 (raw file):
Previously, laurentgo (Laurent Goujon) wrote…
This causes all the XML files across all modules to be checked once by the top level module and then latter by the owning module. Tbh, it feels too broad, and maybe we should only check things under
${project.basedir}/src
Done.
b7b6142 to
d098f30
Compare
|
As discussed in the stand-up, I've re-added the Checkstyle plugin but removed the "pure code formatting" checks from the checkstyle-configs. Nits:
|
.idea/copyright/copy.xml
Outdated
| <component name="CopyrightManager"> | ||
| <copyright> | ||
| <option name="notice" value="Copyright (C) 2020 Dremio Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License." /> | ||
| <option name="notice" value="Copyright (C) &#36;today.year Dremio Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License." /> |
There was a problem hiding this comment.
does this mean once a year we will have a big diff which re-writes all headers or we will have a mixed copywrite year depending on when the source was created
There was a problem hiding this comment.
Nope. It just inserts the current year, when the license is not present.
A "new year" has no effect. However, if you change the copyright text, it will complain.
There was a problem hiding this comment.
so we will have a mix of 2020, 2021 etc as the copyright year right? That sounds odd to me. Most repos have Copyright <year> or Copyright <year1>-<year2> right? And they are always uniform across the repo.
32c2633 to
fe49a23
Compare
ea0b3e2 to
f5d45bf
Compare
Checkstyle is still used to check certain code patterns. Formats `*.java`, `*.scala` and `*.xml` files. Does not format markdown or typescript or python or CSS or HTML files. For typescript: it feels easier to add some tslint/eslint into the node build process. For python: it feels easier to add (actually: leave) it to the python linter (or flake8 or whatever). CSS/HTML: there's not much for format right now, and Eclipse WTP fails to format typescript-ish CSS files. Spotless can use Prettier, but it does an `npm install` for every module, starts a prettier server and shuts it down - takes quite long. Allow `-DskipSpotless`
https://github.com/diffplug/spotless/tree/main/plugin-maven
This change is