Skip to content

Commit 64d776a

Browse files
markiantornojamesagnew
markiantorno
and
jamesagnew
authored
Checkstyle cleanup (#4501)
* cleaning up checkstyle files * added exlusions for files at base project level so checkstyle doesn't error out * duplicate error code from merge * One more fix for #4467 * changing lifecycle goal for all module checkstyle check * moving checkstyle to base pom file, changing exectution phase on base check, cleaning dependency, resolving duplicate error code * wip * trying to figure out why pipeline cannot copy files * removing modules that don't actually need to be built. * I messed up the version * missing model --------- Co-authored-by: James Agnew <[email protected]>
1 parent 9665476 commit 64d776a

File tree

31 files changed

+365
-531
lines changed

31 files changed

+365
-531
lines changed

‎azure-pipelines.yml

-4
Original file line numberDiff line numberDiff line change
@@ -80,16 +80,12 @@ stages:
8080
module: hapi-fhir-server-mdm
8181
- name: hapi_fhir_server_openapi
8282
module: hapi-fhir-server-openapi
83-
- name: hapi_fhir_serviceloaders
84-
module: hapi-fhir-serviceloaders
8583
- name: hapi_fhir_caching_api
8684
module: hapi-fhir-serviceloaders/hapi-fhir-caching-api
8785
- name: hapi_fhir_caching_caffeine
8886
module: hapi-fhir-serviceloaders/hapi-fhir-caching-caffeine
8987
- name: hapi_fhir_caching_guava
9088
module: hapi-fhir-serviceloaders/hapi-fhir-caching-guava
91-
- name: hapi_fhir_caching_testing
92-
module: hapi-fhir-serviceloaders/hapi-fhir-caching-testing
9389
- name: hapi_fhir_spring_boot_autoconfigure
9490
module: hapi-fhir-spring-boot/hapi-fhir-spring-boot-autoconfigure
9591
- name: hapi_fhir_spring_boot_sample_server_jersey

‎hapi-deployable-pom/pom.xml

+4-29
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1-
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
2-
<modelVersion>4.0.0</modelVersion>
1+
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
2+
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
3+
<modelVersion>4.0.0</modelVersion>
34

4-
<parent>
5+
<parent>
56
<groupId>ca.uhn.hapi.fhir</groupId>
67
<artifactId>hapi-fhir</artifactId>
78
<version>6.5.0-SNAPSHOT</version>
@@ -13,9 +14,6 @@
1314

1415
<name>HAPI FHIR - Deployable Artifact Parent POM</name>
1516

16-
<dependencies>
17-
</dependencies>
18-
1917
<build>
2018
<plugins>
2119
<plugin>
@@ -120,27 +118,6 @@
120118
</ignoredResourcePatterns>
121119
</configuration>
122120
</plugin>
123-
<plugin>
124-
<groupId>org.apache.maven.plugins</groupId>
125-
<artifactId>maven-checkstyle-plugin</artifactId>
126-
<executions>
127-
<execution>
128-
<phase>process-sources</phase>
129-
<goals>
130-
<goal>check</goal>
131-
</goals>
132-
<configuration>
133-
<logViolationsToConsole>true</logViolationsToConsole>
134-
<failsOnError>true</failsOnError>
135-
<suppressionsLocation>${maven.multiModuleProjectDirectory}/src/checkstyle/checkstyle_suppressions.xml</suppressionsLocation>
136-
<enableRulesSummary>true</enableRulesSummary>
137-
<enableSeveritySummary>true</enableSeveritySummary>
138-
<consoleOutput>true</consoleOutput>
139-
<configLocation>${maven.multiModuleProjectDirectory}/src/checkstyle/checkstyle.xml</configLocation>
140-
</configuration>
141-
</execution>
142-
</executions>
143-
</plugin>
144121
</plugins>
145122
</build>
146123

@@ -208,7 +185,6 @@
208185
<execution>
209186
<phase>package</phase>
210187
<goals>
211-
<!--<goal>aggregate-jar</goal>-->
212188
<goal>jar</goal>
213189
</goals>
214190
</execution>
@@ -252,5 +228,4 @@
252228
</build>
253229
</profile>
254230
</profiles>
255-
256231
</project>

‎hapi-fhir-base/pom.xml

+56
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,62 @@
196196
</execution>
197197
</executions>
198198
</plugin>
199+
<plugin>
200+
<groupId>org.apache.maven.plugins</groupId>
201+
<artifactId>maven-antrun-plugin</artifactId>
202+
<version>1.8</version>
203+
<configuration>
204+
<target>
205+
<!--suppress UnresolvedMavenProperty -->
206+
<delete dir="${pom.basedir}/target/" includes="checkstyle*"/>
207+
</target>
208+
</configuration>
209+
<inherited>true</inherited>
210+
<executions>
211+
<execution>
212+
<id>delete-module-cache-file</id>
213+
<phase>validate</phase>
214+
<goals>
215+
<goal>run</goal>
216+
</goals>
217+
</execution>
218+
</executions>
219+
</plugin>
220+
<plugin>
221+
<groupId>org.apache.maven.plugins</groupId>
222+
<artifactId>maven-checkstyle-plugin</artifactId>
223+
<version>${maven_checkstyle_version}</version>
224+
<configuration>
225+
<!--suppress UnresolvedMavenProperty -->
226+
<configLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml</configLocation>
227+
<!--suppress UnresolvedMavenProperty -->
228+
<suppressionsLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle-suppression.xml</suppressionsLocation>
229+
<inputEncoding>UTF-8</inputEncoding>
230+
<consoleOutput>true</consoleOutput>
231+
</configuration>
232+
<inherited>true</inherited>
233+
<executions>
234+
<execution>
235+
<id>hapi-single-module-checkstyle</id>
236+
<phase>validate</phase>
237+
<goals>
238+
<goal>check</goal>
239+
</goals>
240+
</execution>
241+
</executions>
242+
<dependencies>
243+
<dependency>
244+
<groupId>ca.uhn.hapi.fhir</groupId>
245+
<artifactId>hapi-fhir-checkstyle</artifactId>
246+
<version>${project.version}</version>
247+
</dependency>
248+
<dependency>
249+
<groupId>com.puppycrawl.tools</groupId>
250+
<artifactId>checkstyle</artifactId>
251+
<version>${checkstyle_version}</version>
252+
</dependency>
253+
</dependencies>
254+
</plugin>
199255
</plugins>
200256
<resources>
201257
<resource>

‎hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/FhirValidator.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ static List<SingleValidationMessage> buildValidationMessages(List<ConcurrentVali
333333
retval.addAll(messages);
334334
}
335335
} catch (InterruptedException | ExecutionException exp) {
336-
throw new InternalErrorException(Msg.code(1975) + exp);
336+
throw new InternalErrorException(Msg.code(2246) + exp);
337337
}
338338
return retval;
339339
}

‎hapi-fhir-checkstyle/pom.xml

+65
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
<dependency>
1919
<groupId>com.puppycrawl.tools</groupId>
2020
<artifactId>checkstyle</artifactId>
21+
<version>${checkstyle_version}</version>
2122
</dependency>
2223
<dependency>
2324
<groupId>commons-io</groupId>
@@ -34,6 +35,31 @@
3435
</dependency>
3536
</dependencies>
3637

38+
<build>
39+
<pluginManagement>
40+
<plugins>
41+
<plugin>
42+
<groupId>org.apache.maven.plugins</groupId>
43+
<artifactId>maven-checkstyle-plugin</artifactId>
44+
<version>3.2.0</version>
45+
<dependencies>
46+
<dependency>
47+
<groupId>com.puppycrawl.tools</groupId>
48+
<artifactId>checkstyle</artifactId>
49+
<version>${checkstyle_version}</version>
50+
</dependency>
51+
<dependency>
52+
<groupId>ca.uhn.hapi.fhir</groupId>
53+
<artifactId>hapi-fhir-checkstyle</artifactId>
54+
<!-- Remember to bump this when you upgrade the version -->
55+
<version>${project.version}</version>
56+
</dependency>
57+
</dependencies>
58+
</plugin>
59+
</plugins>
60+
</pluginManagement>
61+
</build>
62+
3763
<profiles>
3864
<!-- For releases, we need to generate javadoc and sources JAR -->
3965
<profile>
@@ -75,5 +101,44 @@
75101
</plugins>
76102
</build>
77103
</profile>
104+
<profile>
105+
<id>CI</id>
106+
<build>
107+
<plugins>
108+
<plugin>
109+
<groupId>org.apache.maven.plugins</groupId>
110+
<artifactId>maven-checkstyle-plugin</artifactId>
111+
<version>${maven_checkstyle_version}</version>
112+
<configuration>
113+
<excludes>**/osgi/**/*, **/.mvn/**/*, **/.mvn_/**/*</excludes>
114+
<sourceDirectories>
115+
<!--scan base project directory to include all modules-->
116+
<!--suppress UnresolvedMavenProperty -->
117+
<directory>${maven.multiModuleProjectDirectory}/</directory>
118+
</sourceDirectories>
119+
<!--suppress UnresolvedMavenProperty -->
120+
<configLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml</configLocation>
121+
<!--suppress UnresolvedMavenProperty -->
122+
<suppressionsLocation>${maven.multiModuleProjectDirectory}/hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle-suppression.xml</suppressionsLocation>
123+
<inputEncoding>UTF-8</inputEncoding>
124+
<consoleOutput>true</consoleOutput>
125+
</configuration>
126+
<executions>
127+
<execution>
128+
<id>checkstyle-across-all-modules</id>
129+
<phase>install</phase>
130+
<goals>
131+
<goal>check</goal>
132+
</goals>
133+
</execution>
134+
<execution>
135+
<id>hapi-single-module-checkstyle</id>
136+
<phase>none</phase>
137+
</execution>
138+
</executions>
139+
</plugin>
140+
</plugins>
141+
</build>
142+
</profile>
78143
</profiles>
79144
</project>

‎src/checkstyle/checkstyle_suppressions.xml ‎hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle-suppression.xml

+13-3
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,18 @@
11
<?xml version="1.0"?>
2-
32
<!DOCTYPE suppressions PUBLIC
4-
"-//Puppy Crawl//DTD Suppressions 1.1//EN"
5-
"http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
3+
"-//Checkstyle//DTD SuppressionFilter Configuration 1.2//EN"
4+
"https://checkstyle.org/dtds/suppressions_1_2.dtd">
65

76
<suppressions>
7+
<suppress files="[/\\]test[/\\]" checks="[a-zA-Z0-9]*" />
8+
<suppress files="[/\\]resources[/\\]" checks="[a-zA-Z0-9]*" />
9+
<suppress files="[\\/]target[\\/]" checks="[a-zA-Z0-9]*" />
10+
<suppress files="[\\/]hapi-fhir-docs[\\/]" checks="[a-zA-Z0-9]*" />
11+
<suppress files="[\\/]hapi-fhir-checkstyle[\\/]" checks="[a-zA-Z0-9]*" />
12+
<suppress files="[\\/]hapi-fhir-jpaserver-test-utilities[\\/]" checks="[a-zA-Z0-9]*" />
13+
<suppress files="[\\/]hapi-tinder-plugin[\\/]" checks="[a-zA-Z0-9]*" />
14+
<suppress files="[\\/]osgi[\\/]" checks="[a-zA-Z0-9]*"/>
15+
816
<!-- Name suppressions -->
917
<suppress files="Base64BinaryDt\.java" checks="AbstractClassName"/>
1018
<!-- TODO KHS cdr instantiates these. Remove them once cdr has been fixed. -->
@@ -16,4 +24,6 @@
1624
<suppress files="RequestDetails\.java" checks="AbstractClassName"/>
1725
<suppress files="RestfulClientFactory\.java" checks="AbstractClassName"/>
1826
<suppress files="MatchUrlService\.java" checks="AbstractClassName"/>
27+
<suppress files="BaseController\.java" checks="AbstractClassName"/>
28+
1929
</suppressions>

‎hapi-fhir-checkstyle/src/checkstyle/hapi-base-checkstyle.xml

+41
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?xml version="1.0"?>
2+
<!DOCTYPE module PUBLIC
3+
"-//Puppy Crawl//DTD Check Configuration 1.3//EN"
4+
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
5+
6+
<module name="Checker">
7+
8+
<property name="severity" value="error"/>
9+
<property name="charset" value="UTF-8"/>
10+
<property name="fileExtensions" value="java, properties, xml, js, json"/>
11+
12+
<module name="TreeWalker">
13+
<!-- Run custom HapiErrorCodeCheck to find duplicate error codes -->
14+
<module name="ca.uhn.fhir.checks.HapiErrorCodeCheck"/>
15+
<!-- Throw error if any FIX ME is left in the code -->
16+
<module name="TodoComment">
17+
<!-- The (?i) below means Case Insensitive -->
18+
<property name="format" value="(?i)FIXME"/>
19+
</module>
20+
21+
<module name="RegexpSinglelineJava">
22+
<property name="format" value="System\.out\.println"/>
23+
<property name="ignoreComments" value="true"/>
24+
</module>
25+
<module name="RegexpSinglelineJava">
26+
<property name="format" value="org\.jetbrains\.annotations\.NotNull"/>
27+
</module>
28+
<module name="RegexpSinglelineJava">
29+
<property name="format" value="org\.jetbrains\.annotations\.Nullable"/>
30+
</module>
31+
<!-- Should always use the Spring transactional interface, per: https://stackoverflow.com/questions/26387399/javax-transaction-transactional-vs-org-springframework-transaction-annotation-tr -->
32+
<module name="RegexpSinglelineJava">
33+
<property name="format" value="javax\.transaction\.Transactional"/>
34+
<property name="message"
35+
value="Wrong @Transactional annotation used, use instead: org.springframework.transaction.annotation.Transactional"/>
36+
</module>
37+
<module name="AbstractClassName">
38+
<property name="format" value="^(Base|Abstract).+$"/>
39+
</module>
40+
</module>
41+
</module>

‎hapi-fhir-checkstyle/src/main/java/ca/uhn/fhir/checks/HapiErrorCodeCheck.java

+34-16
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
package ca.uhn.fhir.checks;
22

3-
import com.puppycrawl.tools.checkstyle.StatelessCheck;
3+
import com.puppycrawl.tools.checkstyle.FileStatefulCheck;
44
import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
55
import com.puppycrawl.tools.checkstyle.api.DetailAST;
66
import com.puppycrawl.tools.checkstyle.api.TokenTypes;
@@ -9,15 +9,13 @@
99

1010
import java.util.HashMap;
1111
import java.util.Map;
12+
import java.util.Set;
1213

13-
/**
14-
* mvn -P CI checkstyle:check
15-
*/
16-
@StatelessCheck
14+
@FileStatefulCheck
1715
public final class HapiErrorCodeCheck extends AbstractCheck {
1816
private static final Logger ourLog = LoggerFactory.getLogger(HapiErrorCodeCheck.class);
1917

20-
private static final Map<Integer, String> ourCodesUsed = new HashMap<>();
18+
private static final ErrorCodeCache ourCache = ErrorCodeCache.INSTANCE;
2119

2220
@Override
2321
public int[] getDefaultTokens() {
@@ -42,10 +40,6 @@ public void visitToken(DetailAST ast) {
4240
}
4341

4442
private void validateMessageCode(DetailAST theAst) {
45-
// TODO KHS this should be done in the checkstyle plugin pom config, not here
46-
if (getFileContents().getFileName().contains("/generated-sources/")) {
47-
return;
48-
}
4943
DetailAST instantiation = theAst.getFirstChild().getFirstChild();
5044
// We only expect message codes on new exception instantiations
5145
if (TokenTypes.LITERAL_NEW != instantiation.getType()) {
@@ -68,14 +62,13 @@ private void validateMessageCode(DetailAST theAst) {
6862
DetailAST numberNode = msgNode.getParent().getNextSibling().getFirstChild().getFirstChild();
6963
if (TokenTypes.NUM_INT == numberNode.getType()) {
7064
Integer code = Integer.valueOf(numberNode.getText());
71-
if (ourCodesUsed.containsKey(code)) {
65+
if (ourCache.containsKey(code)) {
7266
log(theAst.getLineNo(), "Two different exception messages call Msg.code(" +
73-
code +
74-
"). Each thrown exception throw call Msg.code() with a different code. " +
75-
"Previously found at: " + ourCodesUsed.get(code));
67+
code + "). \nEach thrown exception must call Msg.code() with a different code. " +
68+
"\nPreviously found at: " + ourCache.get(code));
7669
} else {
77-
String location = getFileContents().getFileName() + ":" + instantiation.getLineNo() + ":" + instantiation.getColumnNo() + "(" + code + ")";
78-
ourCodesUsed.put(code, location);
70+
String location = getFilePath() + ":" + instantiation.getLineNo() + ":" + instantiation.getColumnNo() + "(" + code + ")";
71+
ourCache.put(code, location);
7972
}
8073
} else {
8174
log(theAst.getLineNo(), "Called Msg.code() with a non-integer argument");
@@ -110,5 +103,30 @@ private DetailAST getMsgNodeOrNull(DetailAST theNode) {
110103
}
111104
return null;
112105
}
106+
107+
public enum ErrorCodeCache {
108+
INSTANCE;
109+
110+
private static final Map<Integer, String> ourCodesUsed = new HashMap<>();
111+
112+
ErrorCodeCache() {
113+
}
114+
115+
public boolean containsKey(Integer s) {
116+
return ourCodesUsed.containsKey(s);
117+
}
118+
119+
public String get(Integer i) {
120+
return ourCodesUsed.get(i);
121+
}
122+
123+
public String put(Integer code, String location) {
124+
return ourCodesUsed.put(code, location);
125+
}
126+
127+
public Set<Integer> keySet() {
128+
return ourCodesUsed.keySet();
129+
}
130+
}
113131
}
114132

0 commit comments

Comments
 (0)