-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance PMD rules #2753
Enhance PMD rules #2753
Conversation
f6bff61
to
6d87ba7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me but you should get a second opinion on the rules you've selected.
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java
Outdated
Show resolved
Hide resolved
...talk-http-netty/src/test/java/io/servicetalk/http/netty/RetryingHttpRequesterFilterTest.java
Show resolved
Hide resolved
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/StreamObserverTest.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/TestServiceStreaming.java
Outdated
Show resolved
Hide resolved
...nal/src/main/java/io/servicetalk/http/router/jersey/internal/BufferPublisherInputStream.java
Show resolved
Hide resolved
...alk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/NoopAsyncContextProvider.java
Show resolved
Hide resolved
servicetalk-http-api/src/test/java/io/servicetalk/http/api/AbstractHttpHeadersTest.java
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java
Show resolved
Hide resolved
Other than the comments, looks good to me. Admittedly I did not read every exclude rule in detail, but I think if we want to enable more we should do in follow-up PRs since this one is quite big already and naturally touches a lot of files. |
Those that are left excluded were quite noisy and didn't provide much value. I'm happy to exclude more rules and revert changes if anything looks unreasonable. |
Motivation:
Currently,
pmd/basic.xml
defines explicit set of rules to validate. With thisstrategy we miss all new rules that PMD adds overtime.
Modifications:
pmd/basic.xml
to include all rules by default but exclude thosethat are not providing much value;
Result:
More coverage during automated code validation that will reduce nit-picking
comments during PR reviews.