On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <k...@puppetlabs.com> wrote:
> HI all,
>
> We'd like to start using static analysis against the puppet code base both
> to catch certain classes of coding errors and to enforce best coding
> practices. Those are laudable goals of course, but there is plenty of room
> for opinions on what qualifies. This email is a request to solicit some
> opinions :)
>
> To kick the discussion off: at this point, we're leaning toward using
> rubocop for static analysis, identifying a set of checkers ('cops' in
> rubocop lingo) and then setting up some CI integration, either in travis-ci
> or houndci, to enforce those cops against PRs.
>
>
One thing that is always important with these kinds of static checkers is
the ability to turn them off when they aren't wanted or go wrong. I just
checked and it is possible to do this with robucop (
https://github.com/bbatsov/rubocop#disabling-cops-within-source-code),
which makes me much more confident in using it.
For the build and test pipeline integration, I think it would be great to
have it as part of the PR process. However, since every once in a while we
have things enter that don't go through a PR (normally quick little fixes
and such), then we should also have this running in Jenkins.
I don't think that it should be executed as part of "rake spec", since that
would slow it down for us whenever we run them, and it is really one of
those things that once we get into the habit of abiding by the rules, we
very rarely actually break them. What I've done before is have a separate
job in Jenkins for running static checks. It gets kicked off after the spec
tests passed and in parallel with the next stage after specs (in our case,
that is packaging).
> Rahul Gopinath has put together a PR with an initial proposal of 'cops' we
> might use:
>
> https://github.com/puppetlabs/puppet/pull/2855
>
> There's some initial discussion in that PR but the tldr of the proposal is
> to enable these cops:
>
> Lint/UnreachableCode
> Lint/ConditionPosition
> Lint/UselessComparison
> Lint/LiteralInterpolation
> Lint/ElseLayout
>
>
>From the changes in the PR from those cops, I think they all look like good
ones to turn on.
> and then there's been some discussion on the PR around these two cops:
>
> Style/AndOr
> Lint/AssignmentInCondition
>
> Each of those two checks catch coding patterns which both are a source of
> some bugs and, at the same time are idiomatic in certain cases. So there's
> room for discussion on those two.
>
>
The only idiomatic part of the and/or one that we'd get rid of is failure
statements: do_something() or raise "Noooo!!!". I'm willing to drop that
for the slightly more wordy:
if !do_something()
raise "Nooooo!!!!"
end
In fact, I often like that construction better because it is more future
proof. If it turns out that you need to add extra actions in the error
case, then there isn't any need to perform refactor first.
> And then there are a *bunch* more cops for a variety of style/lint checks
> which we could consider enabling in addition to the above. There's some
> documentation of the various cops in the rubocop yaml files at:
>
> https://github.com/bbatsov/rubocop/tree/master/config
>
>
I think we should avoid any purely formatting cops. Instead we should
figure out a common formatting utility to use and just take code formatting
out the path of a developer's whims. On the other cops, any cop that says
"this should ****usually**** be ok" I think we can just completely take out
of consideration. If it is wrong often enough that the description says
that, then the codebase will be littered with turning off the check.
> So, thoughts?
>
> Kylo
>
> --
> Kylo Ginsberg
> k...@puppetlabs.com
>
> *Join us at PuppetConf 2014 <http://www.puppetconf.com/>, September
> 20-24 in San Francisco*
> *Register by July 31st to take advantage of the Early Bird discount
> <https://puppetconf2014.eventbrite.com/?discount=EarlyBird> **—**save
> $249!*
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to puppet-dev+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/puppet-dev/CALsUZFHmU%2B8aAHLNV3nu5HK98d4%2BEw0Ez-GBJZHpTD7gddSSJA%40mail.gmail.com
> <https://groups.google.com/d/msgid/puppet-dev/CALsUZFHmU%2B8aAHLNV3nu5HK98d4%2BEw0Ez-GBJZHpTD7gddSSJA%40mail.gmail.com?utm_medium=email&utm_source=footer>
> .
> For more options, visit https://groups.google.com/d/optout.
>
--
Andrew Parker
a...@puppetlabs.com
Freenode: zaphod42
Twitter: @aparker42
Software Developer
*Join us at PuppetConf 2014 <http://www.puppetconf.com/>, September
22-24 in San Francisco*
*Register by May 30th to take advantage of the Early Adopter discount
<http://links.puppetlabs.com/puppetconf-early-adopter> **—**save $349!*
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/puppet-dev/CANhgQXt9FzMvdwY1g6iSuEuWWCOY5e3Qw%2Bponb86ve%3DN43%3DaiQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
