Upgrade checkstyle to fix CVE-2019-9658 vulnerability

[kaspersorensen]

OMG. I'm getting tired of checkstyle, and would vote to remove it on the basis of it's annoying function alone, but now I'm just going to remove it because it's apparently completely broken if you want it without vulnerabilities.

[LosD]

Hmmmm. I was more than a little fond of Checkstyle, as it kills quite a bit of stupid behaviour without having to discuss it.

Isn't there some sort of upgrade document if it changed that much?

[kaspersorensen]

I've never fully understood how it's configured, so doing the upgrade is very painful. I guess I can be pursuaded to keep checkstyle if someone else wants to "own" our configuration for it, but for my many last contributions I've not been very appreciative of it :-) This may be a deroute of the discussion, but I would prefer "full file format on save" (using our Eclipse or IntelliJ formatters) as a policy rather than checkstyle.

[LosD]

Full format on save first of all limits the tools that can be used, can't really be checked, and can't protect against many other classes of issues.

In my opinion, we should have much more like this, and it should be much stricter, especially outside simple formatting.

[LosD]

OTOH I don't really care about the tool, but I'd mind if changing it was used as an excuse to lax the rules.

(By the way, I'm not really contributing enough anymore that I should have any real say here, so mostly take this at "my 2 cents"-level 🙂)

[kaspersorensen]

It's not that I don't think checkstyle is valuable at all. But to me it feels like the sort of thing you implement in a big organization to keep control over everything. If we had tons of contributors then I would see the benefit here too. But with our relatively low amount of contributors, I'd favor making it easy to contribute over strictness and control. I think/hope that people who contribute will find it more motivating if they first build their contribution and then during review they might get a few pointers as to how to make it better.

[LosD]

Hmmmm, I usually see it the opposite way. If it's a simple set of rules that's enforced automatically, it's usually easier to accept than being told "that's wrong". And most IDEs has plugins that will tell you right away, so no trial and error (and all the delays that comes with people waiting for replies).

Of course, a contributor guide would help with getting those plugins installed.

[kaspersorensen]

https://github.com/datacleaner/DataCleaner/blob/master/CONTRIBUTE.md :-)

[LosD]

Heh! Then we just need to add a section to that, and possibly add a PR template that refers to it (if we already have that, it's SERIOUSLY too long I contributed anything...) :smile:

[kaspersorensen]

Having a PR template is an excellent idea. Please go ahead with that. Now I'm gonna go and merge this :-P

[LosD]

Errr... The template was alongside proper checkstyle in the code, to make sure people had also run it locally, definitely not instead of.

This is a big step backwards

[kaspersorensen]

I thought you meant that the PR template would point out the contributor guidelines.

Anyway, bringing checkstyle back in is pretty easy. But I firmly believe that we need to be more critical about what it should be allowed to dictate. The way it was set up until now was IMO very strict and caused a lot more annoyance than it should. I agree on it checking certain things (certainly all the trivial and ubiquitous code conventions) but I go crazy when it interferes with e.g. abbreviations in variable naming, which I think is very sensible and very common. All this said, maybe we should have an issue to discuss what rules we want to see and which we don't.

[LosD]

That was the best rule! :smile:

Anyway, we can do that. I for one see no value in spending review time looking for things that should never have been committed in the first place (hence the PR template. One of the checklist questions should be "did you run checkstyle (or whatever tool we end up with)?"

What I would love is some Github app that points it out directly, instead of burying it deep inside a Travis build error. We looked for something a while ago, and didn't really find anything that covered our needs, but maybe that has changed now. A quick look shows, something like Hound or codebeat. looks decent, and is free for open source. I just hope one of them also be used before committing (or can use e.g. Checkstyle rules)