cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rafael Weingärtner <rafaelweingart...@gmail.com>
Subject Re: Checkstyle / code style / reformatting
Date Fri, 29 Sep 2017 10:56:26 GMT
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 <marco@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.hoogland@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 <marco@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message