Let user choose which version of checkstyle to use

[2is10]

Our company uses both this plugin (checkstyle-idea) and maven-checkstyle-plugin.

The maven plugin lets us configure which version of checkstyle to use, but this plugin doesn’t. As a result, when this plugin updates, our IDEA checkstyle results can get out of sync with our maven checkstyle results (which can break the build). For example, this happened recently when this plugin updated from checkstyle 6.11 to 6.12, which has buggy lambda support.

I don’t know much about IDEA plugin architecture or development patterns. Would it be feasible to let the user configure which version of checkstyle to use? Does every plugin need to be completely self-contained (bundled with all of its dependencies), or can it have dynamic dependencies that are somehow resolved and downloaded when the plugin starts?

Seems like this more flexible approach might make your life a little easier too, since you wouldn’t have to keep chasing the latest checkstyle release with one of your own.

[jshiell]

I appreciate the need, but unfortunately it's unlikely to happen. There are a few reasons:

• While we could move it to being an dynamic dependency (for example, we already have dynamic loading for Checkstyle plugin JARs), this would mean the user would have to not only import the Checkstyle JAR but also the many JARs CS depends on (11 for 6.12.1).
• Another option would be to package multiple versions of CS with IDEA, but this would bulk up the JAR a lot (4-5Mb per version) and have the joys of maintaining multiple APIs.
• And the multiple APIs is the biggest problem - most of the pain comes not from updating the CS dependency, but from subtle changes within. In particular, error handling has changed a few times in the past few releases, and as Checkstyle makes no guarantees as to what will emerge from a syntactically invalid file (e.g. during editing) we need to ensure that we handle the cases for the current version.

Thus I fear at present it's unlikely that I'll progress along this line. Sorry.

[mkordas]

@2is10

6.12, which has buggy lambda support

Can can you share more details? Is there issue opened in Checkstyle repo? We try to fix such problems ASAP.

[daviderickson]

https://github.com/checkstyle/checkstyle/issues/281

[2is10]

@mkordas, thanks for asking.

Here are two examples of what strikes me as buggy lambda behavior in 6.12.1:

1) In this example, checkstyle is requiring two spaces between a lambda and a subsequent chained method call.

        model.setFoos(bar.getFoos().stream().map(f -> {
            try {
                return baz.save(f);
            } catch (IOException ex) {
                throw new RuntimeException(ex);
            }
        })  .collect(Collectors.toSet()));

Without them, the error message (in IntelliJ) reads:

'.' have incorrect indentation level 10, expected level should be 12.

2) In this example, the second lambda expression block would look better if it were indented at the same level as the first one, but that generates an error.

    return Collector.of(ArrayList::new, (result, foo) -> {
        final Matcher match = PATTERN.matcher(foo);
        if (match.find()) {
            // ...
        }
        if (aggregator[0] != null) {
            aggregator[0].add(foo);
        }
    }, (result1, result2) -> {
            throw new UnsupportedOperationException("Don't run in parallel");
        });

[2is10]

@jshiell Thanks for the prompt and thoughtful response. You’re in a better position to evaluate feasibility and tradeoffs. I didn’t see this capability directly requested in any other issues, so I thought it worthwhile to propose.

Going forward, if it’s not too much trouble, I’d ask that you please include an installable .zip file for every release, in case users update and then decide to back-pedal. Right now I don’t see an easy way to get back to release 4.20.1, for example.

Thanks for all of your work on this plugin!

[jshiell]

@2is10 thanks for your kind words! Suggestions are always welcome, and you never know - maybe I'll have a flash of insight and come up with a way to accomplish it.

I have been very inconsistently attaching binary zip files to the GitHub releases; now I know they're useful I'll try and do that for every release. You can also download the zips from Jetbrains' repo - although you'll probably have to click the show all updates link in the bottom right.

On the Mac you can list unzip those over the original install in ~/Library/Application Support/IntelliJIDEA14; I believe it's identical on Linux/Windows once you've found the appropriate place to do so.

[2is10]

Thanks for the links. I didn’t realize JetBrains made archived releases available on their website.

[mkordas]

@2is10 Thanks for the examples. Can you submit them as issues to https://github.com/checkstyle/checkstyle/issues along with configuration of indentation check that you use?

[tsjensen]

I have fiddled with this problem for a while, and believe that the "package multiple versions" variant you proposed should be doable. We would offer a dropdown box in the config panel that allows the user to select a Checkstyle version: Indeed, there are several problems to solve.

Firstly, the plugin package size. We mitigate that by including not the checkstyle-all JARs, but the individual JARs. I implemented this in Gradle and it turns out that the total space taken up by all required JARs of every feasible version since Checkstyle 6.2 is only 42 MB (for 30 versions of Checkstyle). These JARs go into a separate checkstyle folder instead of /lib so they don't pollute the classpath. Gradle creates an info file with the class paths required for each Checkstyle version, so we can set up a custom classloader in the plugin to correctly load (and discard) any version of Checkstyle. The main effort would be to introduce a new abstraction layer into the plugin (e.g. a CheckstyleProjectService) which encapsulates all Checkstyle interaction. In order to make sure that this does not create added maintenance effort in the future, we would write a lot of unit tests for the new abstraction layer and test it against the supported Checkstyle versions during the build. I apply this technique successfully for Checkstyle Addons (example). It may require a few iterations to handle the subtle differences between versions correctly, but it would certainly be worth trying!

You can take a look at my investigations so far as they have come on this branch. I would try to get this working if you support the idea. It would be a major code change though.

Let me know what you think!

[jshiell]

That's awesome - thank you! You seem to have covered off a lot of the questions on this, and found a nice vector of attack as well.

The abstractions against multiple versions should be relatively simple (at present, anyway) - to the best of my memory, the API has changed in a compilation-breaking manner only once in the last ten years, and the major risks of a new version come in how exceptions are propagated from Checkstyle. That code is getting reasonably robust, so as a starting point we could probably switch easily between 6.x and 7.x without great problems.

There's a minor question as to which versions to include - for instance, do you need 7.1, 7.1.1 and 7.1.2 or just 7.1.2? - but this really is nit-picking, and a question which is probably best answered in the field.

My personal interest in this is pretty low, as I haven't even used this plugin in anger for more than three years now, and as such really only devote maintenance to it. So I'm thrilled that it's obviously of enough use to you to put this level of effort in, and I'm very happy to do everything I can to support that. And as this thread suggests, it's obviously of interest to others as well!

So thank you again, and assuming you are willing to devote the time to fleshing this out I'd be very happy to offer my support 😄

[tsjensen]

Great, that's good to hear. Thanks especially for the offer of support - I shall come back to that. :smile: Which versions exactly to support is indeed something we can decide later, as we'd only have to change a config file entry. The Checkstyle Compatibility Matrix may provide some details that can help us make the decision. It will take a lot of time to implement everything, several months at least, because I'll have to get it done with only a few hours per week to spare. It would be very helpful if you didn't do any other major refactorings (or package renames) during that time, so that I don't get killed by the merge effort. One more thing: Could you share your code formatter settings with me, or maybe put them up on the README.md's development section? Thanks!

[jshiell]

I've no plans for any big refactors, and if that does change I'll put them off - nothing quite like merge hell to kill enthusiasm!

Curiously enough, I've never use Checkstyle with the plugin. I just stick with the standard Java layout (as default in IntelliJ) on the basis the best layout is the one everyone defaults to. I do like braces on all blocks though (as I've been bitten by prod bugs where people have made mistakes with unbraced ifs), descriptive naming and I tend to wrap about the 120 character where appropriate, mostly as I tend to run with tests and code side-by-side these days. However, the code is inconsistent - especially in the almost complete absence of tests - as a lot of it was written some time ago (some of it is over 10 years old now), when I knew even less about software development than I do now 😄

[tsjensen]

I meant in XML form, for me to import, via Settings → Editor → Code Style → Java → Manage... → Export. ;-) That would be nice in order to be sure that any changes are really changes, and not just differences in formatter configuration. Thanks!

[jshiell]

Sorry, completely misunderstood 😄 Attached below, this is dumped straight from my project, and so should be legit.

It's zipped, as GitHub refuses to host XML, bless them.

styledump.xml.zip

[tsjensen]

Here's a short status update, just to let you know that there is progress. :snail: :smiley: I have reached a major milestone on my branch, because the new abstraction layer is in place, and stuff compiles again. Unit tests are green. Next steps:

• Add lots more unit tests to the Checkstyle service layer (csaccessTest). Coverage must be high enough to be certain that changes to the Checkstyle API can't break our plugin.
• Extend the build so that the csaccessTests are run against every supported Checkstyle version.

When those steps are done, I'll let you know again. Then, I may need your support in making sure that everything still works as before. Now enjoy the upcoming holidays! :christmas_tree: :santa:

[jshiell]

Sounds like you've made substantial progress - great work! Happy Christmas 🎅

On 18 Dec 2016, at 13:36, Thomas Jensen notifications@github.com wrote:

Here's a short status update, just to let you know that there is progress. 🐌 😃 I have reached a major milestone on my branch, because the new abstraction layer is in place, and stuff compiles again. (One unit test still fails, but that seems to have its root cause in the IntelliJ, not the Checkstyle area.) Next steps are:

Add lots more unit tests to the Checkstyle service layer (csaccessTest). Coverage must be high enough to be certain that changes to the Checkstyle API can't break our plugin. Extend the build so that the csaccessTests are run against every supported Checkstyle version. When those steps are done, I'll let you know again. Then, I may need your support in making sure that everything still works as before. Now enjoy the upcoming holidays! 🎄 🎅

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jshiell]

This has now been released in 5.0.0, which has just been uploaded.

All credit goes to @tsjensen - awesome stuff 😄