I am in favor of that. I only have a comment to add. We are already checking code style when validating PRs. So, it is only a matter of adding more checks to the check-style plugin that we are already using.
On Fri, Sep 29, 2017 at 5:36 AM, Marc-Aurèle Brothier <ma...@exoscale.ch> wrote: > The checkstyle rules can be imported in pretty much any IDE to help during > coding and to display the errors immediately. > To speed up the detection of checkstyle errors, we could edit the post > commit hooks to run a mvn command only for the checkstyle rules, and then > do the usual stuff. > > I'm not in favor to alter the code after being approved. The PRs should be > "correct" on all points: code style, functionality, tests, doc > > On Fri, Sep 29, 2017 at 10:19 AM, Daan Hoogland <daan.hoogl...@gmail.com> > wrote: > > > I am a fan of convention and think everyone should have some. Strict > > enforcing of non-functionals, I'm not to big on. I see what you want to > > achieve here but am reluctant not to -1 any strictness. If we do it with > a > > post-commit-hook (or if such a thing exists a post-merge-hook) we will > > allow for code to be altered after being approved, won't we? > > > > On Fri, Sep 29, 2017 at 9:44 AM, Marc-Aurèle Brothier <ma...@exoscale.ch > > > > wrote: > > > > > Hi everyone, > > > > > > Would you think it's worth tightening the checkstyle rules and run a > > > reformatting pass on the code base to align everything slowly? I know > it > > > will hide changes using the blame functionality, and would force people > > to > > > edit some PR. Is the trade-off worth it for you? > > > > > > The idea would be to add one by one those extra checkstyle rules, and > do > > > the code change/reformat accordingly each time with one rule in one > PR. A > > > new rule should first be accepted by the community before starting on > the > > > PR to do the changes (otherwise it might be a lot of wasted time). When > > > merged, a new rule can be added. > > > > > > The code style that bothers me most right now for example is blocks > > without > > > braces, so my first proposal would be: > > > > > > <module name="NeedBraces"> > > > <property name="allowSingleLineStatement" value="true"/> > > > </module> > > > > > > I have a lot more to add, but as said, one at a time ;-) > > > > > > The list of rules: http://checkstyle.sourceforge.net/checks.html > > > The current rules: > > > https://github.com/apache/cloudstack/blob/master/tools/ > > > checkstyle/src/main/resources/cloud-style.xml > > > > > > What do you think? > > > > > > Marc-Aurèle > > > > > > > > > > > -- > > Daan > > > -- Rafael Weingärtner
