STORM-3425: Add module UnusedImports to checkstyle configuration

[krichter722]

No description provided.

[srdo]

I'm a little unsure we want this check. Unused imports can be a little noisy, but (IMO) don't impact readability much, and I think this could be pretty annoying when working with the code (e.g. commenting out some stuff).

[krichter722]

Most IDEs can be configured to remove unused imports automatically. Furthermore they offer plugins for checkstyle which warn on-the-fly during typing. For commenting out there's probably no good solution; for me personally it's a rare case in my daily routine. However, I don't want to push this. I assumed that silence on the created issue meant go ahead. This can be closed.

[srdo]

I don't have a strong opinion on this, I'd just like to avoid checkstyle becoming too much of a hassle for contributors. I've seen other projects go what I would consider overboard with checks that I don't think contribute much to code quality (e.g. checking for whitespace in empty lines), and it adds a bunch of busywork when making a contribution.

I'm not against trying this. We can always remove the check again if it turns out to be annoying. You will need to fix any violations before we can merge this though.

By the way, I missed this in your earlier PR to remove JavadocMethod, but please update the comment here https://github.com/apache/storm/blob/master/storm-checkstyle/src/main/resources/storm/storm_checkstyle.xml#L28. Otherwise we will probably accidentally overwrite your changes next time we update Checkstyle.

[krichter722]

It's better to discard it if you don't feel the need. I did the checkstyle changes as a sort of meditation from which I have no gain except having done it.

By the way, I missed this in your earlier PR to remove JavadocMethod, but please update the comment here https://github.com/apache/storm/blob/master/storm-checkstyle/src/main/resources/storm/storm_checkstyle.xml#L28. Otherwise we will probably accidentally overwrite your changes next time we update Checkstyle.

I read this multiple times and I don't get what should be changed and why :) How can changes be overwritten?

[srdo]

When we update the Checkstyle version, we need to also update the storm_checkstyle.xml. Storm_checkstyle.xml is based on google_checkstyle.xml, and makes a few adjustments to that file. When Checkstyle publishes a new version of Checkstyle, they also publish a new google_checks.xml.

This means that when Storm updates to a new Checkstyle version, we go and get the new google_checks.xml, and overwrite the storm_checkstyle.xml. We then make the couple of modifications to it that are outlined in the comment I linked.

This is obviously an error prone and bad way of making such adjustments, but Checkstyle currently doesn't support extending/overriding existing check files, so we have to get the full google_checks.xml and adjust it.

[srdo]

Regarding what should be changed: We need to put an extra line at https://github.com/apache/storm/blob/master/storm-checkstyle/src/main/resources/storm/storm_checkstyle.xml#L28 telling the reader that we've removed the JavadocMethod rule.

It's meant as a help for the person updating checkstyle in Storm, so it's obvious to them what they need to change in google_checks.xml to get the new version of storm_checkstyle.xml.

[krichter722]

see #3086