Issue #19 Add style checker #20

MichiruKayo · 2017-10-05T07:20:57Z

--added check style
--fixed check style findings.

--fixed check style findings.

trioletas · 2017-10-05T07:50:08Z

code-analysis/check-style/check-style.xml

@@ -0,0 +1,109 @@
+<?xml version="1.0"?>


that's a lot of style checks..are there any presets available out there that we could simply reference here? just curious

There are 2 presets available :
http://checkstyle.sourceforge.net/style_configs.html Google and Sun, will check how to include them by default

unfortunately it is not possible to have 2 configs . Issue is open in style checker :checkstyle/checkstyle#991

trioletas · 2017-10-05T07:51:07Z

api/src/main/java/lv/ctco/tpl/bff/integration/icndb/jokes/JokeValueModel.java

-    String id;
-    String joke;
-    List<String> categories;
+    private String id;


can't say I like this one: who cares if it's not private? It's just a stupid java bean

trioletas · 2017-10-05T07:52:59Z

api/build.gradle

+  def dir = "${parent.projectDir}/code-analysis/check-style"
+  toolVersion = '8.2'
+  configProperties.configDir = dir
+  configFile = file("$dir/check-style.xml")


Can we use the default file location instead and just drop this line? I would like to reduce boilerplate configs as much as possible, let's stick with convention over configuration as much as possible please

trioletas · 2017-10-05T07:53:39Z

api/build.gradle

@@ -24,10 +24,30 @@ apply plugin: 'org.springframework.boot'

 apply from: 'dotenv.gradle'

+apply plugin: 'checkstyle'
+apply plugin: 'pmd'
+pmdTest.enabled = false


In tests we usually have hard coded strings/ magic numbers, and style check fails in such case..

can we use inline directives to suppress the linting for such cases? tests are also code so we should treat them as such

trioletas · 2017-10-05T07:54:19Z

README.md

+
+#### IntelliJ
+
+- Edit configurations > "Application" > Add task in "Before launch" section "checkstyleMain pmdMain" and place it as first


is that necessary ? can we just use the plugin and be done with that?

If you start using intellij idea Application run/debug then plugin is not executed.. couldn't find how to force it..

Okay let's think about that. But please remove these instructions from the documentation. its unnecessarily cumbersome

trioletas · 2017-10-05T07:54:55Z

README.md

+
+- Edit configurations > "Application" > Add task in "Before launch" section "checkstyleMain pmdMain" and place it as first
+- Additionally you can download "CheckStyle plugin". Then open Settings > Other Settings > Checkstyle. Add path to check-style.xml file 
+  and provide link to check-style folder in "${configDir}". This will display errors in editor, but it wont affect build/run. 


but it won affect build/run

sounds fine by me :) who doesn't love the local build failing due to a missing private modifier?

trioletas · 2017-10-05T07:56:56Z

README.md

+
+#### Gradle
+
+- As default behaviour it is configured to call "checkstyleMain pmdMain" before "bootRun" task is executed


Not sure I like this one, it basically stops me from launching the dev mode unless all my modifiers are private and stuff.. Can we relax this one?

trioletas · 2017-10-05T07:57:17Z

api/build.gradle

+  toolVersion = '8.2'
+  configProperties.configDir = dir
+  configFile = file("$dir/check-style.xml")
+  sourceSets = [] // used to skip tests


trioletas · 2017-10-05T07:57:58Z

api/build.gradle

+pmd {
+  toolVersion = '5.8.1'
+  consoleOutput = true
+  ruleSetFiles = files("${parent.projectDir}/code-analysis/pmd/pmd.xml")


I think we can do a convention over configuration with this one as well. I do recall there being a default directory where pmd looks for its configs

trioletas · 2017-10-05T07:58:37Z

code-analysis/check-style/check-style.xml

+    </module>
+
+    <module name="Header">
+        <property name="header" value="Copyright Swiss Reinsurance Company, Mythenquai 50/60, CH 8022 Zurich. All rights reserved."/>


oops you did it again

please be careful with this stuff

trioletas · 2017-10-05T07:59:23Z

code-analysis/check-style/suppressions.xml

+    "http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
+
+<suppressions>
+    <!--Add exclusion here (e.g <suppress files="JokeResolver.java" checks="MethodLength"/>) -->


Are we trying to teach how to use check-style here? Please drop all unnecessary config files, thanks!

--removed pmd (only 1 check is really needed, not worth adding by default) --cleanup of checks --updated readme

MichiruKayo · 2017-10-06T19:45:29Z

1: Removed pmd checks (not worth it)
2: Based on default checks, reviewed them and removed ones i think are not needed
3: updated read.me
4: config location is required. Default path is <build.gradle.folder>/config/stylecheck/stylecheck.xml, in our case it is api/config/... , dont think adding such folder is good.. maybe better to change it to root build.gradle? ( but i do not know how to configure that)
5: Changed dependendOn to "build" will probably be called during deployment?.. not sure where to attach it, since we do not have many tasks..

trioletas · 2017-10-07T10:43:17Z

4: config location is required. Default path is <build.gradle.folder>/config/stylecheck/stylecheck.xml, > in our case it is api/config/... , dont think adding such folder is good.. maybe better to change it to > > root build.gradle? ( but i do not know how to configure that)

I do not see a problem with the api/config folder. It is a configuration for the api module is it not?

MichiruKayo · 2017-10-08T13:11:19Z

Changed to default location as we discussed offline

trioletas · 2017-10-08T18:20:37Z

Thanks, looks good to me.
@sergeytrasko any comments from your side? Otherwise we're good to go.

--added check style

c9d61ae

--fixed check style findings.

trioletas reviewed Oct 5, 2017

View reviewed changes

trioletas requested a review from sergeytrasko October 5, 2017 07:59

--removed unecessary configs

436da5a

--removed pmd (only 1 check is really needed, not worth adding by default) --cleanup of checks --updated readme

--moved to default location

582ceee

trioletas merged commit dd499a0 into ctco:master Oct 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #19 Add style checker #20

Issue #19 Add style checker #20

MichiruKayo commented Oct 5, 2017

trioletas Oct 5, 2017 •

edited

MichiruKayo Oct 5, 2017

MichiruKayo Oct 6, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017

MichiruKayo Oct 5, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017 •

edited

MichiruKayo Oct 5, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017 •

edited

trioletas Oct 5, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017

trioletas Oct 5, 2017

MichiruKayo commented Oct 6, 2017 •

edited by trioletas

trioletas commented Oct 7, 2017 •

edited

MichiruKayo commented Oct 8, 2017

trioletas commented Oct 8, 2017


		#### IntelliJ

		- Edit configurations > "Application" > Add task in "Before launch" section "checkstyleMain pmdMain" and place it as first


		#### Gradle

		- As default behaviour it is configured to call "checkstyleMain pmdMain" before "bootRun" task is executed

Issue #19 Add style checker #20

Issue #19 Add style checker #20

Conversation

MichiruKayo commented Oct 5, 2017

trioletas Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trioletas Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trioletas Oct 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichiruKayo commented Oct 6, 2017 • edited by trioletas

trioletas commented Oct 7, 2017 • edited

MichiruKayo commented Oct 8, 2017

trioletas commented Oct 8, 2017

trioletas Oct 5, 2017 •

edited

trioletas Oct 5, 2017 •

edited

trioletas Oct 5, 2017 •

edited

MichiruKayo commented Oct 6, 2017 •

edited by trioletas

trioletas commented Oct 7, 2017 •

edited