Update checkstyle and fix violations by tlrx · Pull Request #20909 · elastic/elasticsearch

tlrx · 2016-10-13T12:12:44Z

This PR updates the version of Checkstyle from current 5.9 to 7.1 and fix a bunch of new violations.

A more recent version would allow us to use some new checkstyle modules (like Javadocs).

RedundantModifier is improved and now detects more violations that are fixed by this PR. The changes fall into three categories (thanks @nik9000 for the nice summary):

public methods on non-public classes don't make sense. Remove the modifier and let the method inherit the modifier.
try-with-resources variables should not be declared final because they are implicitly final. final adds no extra information for the reader. I guess that makes them names rather than variables....
enums are alway static so there is no need to declare them static.
final doesn't do anything on anonymous classes because they can't be extended anyway. So we just remove the modifier.
Classes declared inside of interfaces are always static so we shouldn't also declare them as static.

For the record, most of the changes were automated using this small Bash script:

#!/bin/bash
input="/tmp/violations.txt"

# File is composed of lines like:
# /tmp/elasticsearch/src/test/java/org/elasticsearch/action/Action.java:10:9:public
# 
while IFS=':' read -ra ADDR; do
    if [ "${ADDR[3]}" != "" ]; then
        echo "${ADDR[0]}" | xargs sed -i "${ADDR[1]}s/${ADDR[3]} //"
    else 
        echo "Skipping file ${ADDR[0]} line ${ADDR[1]} word ${ADDR[3]}"
    fi
done < "$input"

Checkstyle was not updated to the latest version 7.1.2 because it reports violations for final parameters in interface methods (checkstyle/checkstyle#3322) and we have a lot of these for good reason.

I'm really sorry for the boringness of this kind of pull request.

nik9000 · 2016-10-13T12:37:58Z

I'll reviewing it. I like boring PRs while drinking coffee in the morning.

nik9000 · 2016-10-13T13:03:16Z

we have a lot of these for good reason.

I think if we think our reasons are good we should pop over to the checkstyle project and file an issue asking to be able to disable that portion of the check. Is our reason that we want implementers to declare those parameters final and we like that IDEs do that by default if the parameters are final in the interfaces? Beyond that I can't think of any reason to declare parameters on interface methods final.

nik9000 · 2016-10-13T13:23:49Z

client/rest/src/test/java/org/elasticsearch/client/RestClientIntegTests.java

            httpServer.createContext("/" + pathPrefix + statusCode + uniqueContextSuffix, new ResponseHandler(statusCode));

-        try (final RestClient client =
+        try (RestClient client =


I'd never really thought of this but it makes sense.

tlrx · 2016-10-13T13:32:55Z

Is our reason that we want implementers to declare those parameters final and we like that IDEs do that by default if the parameters are final in the interfaces?

Yes, this is the "good reason" I'm talking about. I didn't check every place but each time such a violation was reported I found a good reason to let the final modifier in place.

I think if we think our reasons are good we should pop over to the checkstyle project and file an issue asking to be able to disable that portion of the check.

Makes sense.

nik9000

All the code changes look good to me. They fall into three categories:

public methods on non-public classes don't make sense. Remove the modifier and let the method inherit the modifier.
try-with-resources variables should not be declared final because they are implicitly final. final adds no extra information for the reader. I guess that makes them names rather than variables....
enums are alway static so there is no need to declare them static.
final doesn't do anything on anonymous classes because they can't be extended anyway. So we just remove the modifier.
Classes declared inside of interfaces are always static so we shouldn't also declare them as static.

nik9000 · 2016-10-13T13:46:07Z

Yes, this is the "good reason" I'm talking about. I didn't check every place but each time such a violation was reported I found a good reason to let the final modifier in place.

Personally I'm not sure that "because implementers should declare them final" is really a good reason and I wouldn't be surprised it the checkstyle folks argued that way, but it is worth asking.

javanna · 2016-10-13T13:49:03Z

Question: why do we have all these violations to fix just because we are upgrading checkstyle? Did the set of applied rules change as well? This all makes sense (I was tired too of seeing all these warnings in my IDE) but it would be good (for next time) to do one category at a time maybe? Can we also update the description of the PR with the nice summary that Nik posted?

nik9000 · 2016-10-13T13:51:40Z

Did the set of applied rules change as well?

The meaning of the RedundantModifier changed. There doesn't look like there is a way to disable each category while we upgrade either which is a pain.

tlrx · 2016-10-13T13:52:56Z

Question: why do we have all these violations to fix just because we are upgrading checkstyle? Did the set of applied rules change as well?

Not really the set of rules but rather the way the rule are working. RedundantModifier check is more "complete" in 7.1 than in 5.9 and that's why it reports new violations.

This all makes sense (I was tired too of seeing all these warnings in my IDE) but it would be good (for next time) to do one category at a time maybe?

Sadly we can't because we can't really configure the granularity of the checks.

Can we also update the description of the PR with the nice summary that Nik posted?

Sure, I should have done that.

javanna · 2016-10-13T13:55:56Z

Sadly we can't because we can't really configure the granularity of the checks.

Maybe we could have fixed the issues one category at a time before updating checkstyle for instance? Not sure though.

Anyways, thanks for fixing these!

nik9000 · 2016-10-13T13:58:26Z

Maybe we could have fixed the issues one category at a time before updating checkstyle for instance? Not sure though.

Yeah, we certainly could have. If these changes hadn't been super mechanical stuff I could review while leisurely sipping coffee I'd have asked for that. And we should totally keep that as an option for next time!

tlrx · 2017-02-03T14:40:24Z

Closed in favor of #22960

tlrx added 2 commits October 13, 2016 12:31

Update Checkstyle version to 7.1

b553163

Fix violations with checkstyle 7.1

4db79cc

tlrx added >non-issue review v6.0.0-alpha1 labels Oct 13, 2016

tlrx mentioned this pull request Oct 13, 2016

Add checkstyle rule to forbid empty javadoc comments #20881

Merged

nik9000 self-assigned this Oct 13, 2016

nik9000 reviewed Oct 13, 2016

View reviewed changes

nik9000 approved these changes Oct 13, 2016

View reviewed changes

tlrx closed this Feb 3, 2017

tlrx deleted the update-checkstyle-and-fix-violations branch February 3, 2017 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update checkstyle and fix violations#20909

Update checkstyle and fix violations#20909
tlrx wants to merge 2 commits intoelastic:masterfrom
tlrx:update-checkstyle-and-fix-violations

tlrx commented Oct 13, 2016 •

edited

Loading

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

nik9000 Oct 13, 2016

Uh oh!

tlrx commented Oct 13, 2016

Uh oh!

nik9000 left a comment

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

javanna commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

tlrx commented Oct 13, 2016

Uh oh!

javanna commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

tlrx commented Feb 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tlrx commented Oct 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

nik9000 Oct 13, 2016

Choose a reason for hiding this comment

Uh oh!

tlrx commented Oct 13, 2016

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

javanna commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

tlrx commented Oct 13, 2016

Uh oh!

javanna commented Oct 13, 2016

Uh oh!

nik9000 commented Oct 13, 2016

Uh oh!

tlrx commented Feb 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tlrx commented Oct 13, 2016 •

edited

Loading