So now putting on my opinionated hat ;)
On Mon, Jul 14, 2014 at 12:04 PM, Andy Parker <a...@puppetlabs.com> wrote:
> On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <k...@puppetlabs.com>
> wrote:
>
>> 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.
>>
>
Yep, +1 on it being in Jenkins also.
> 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).
>
In Rahul's PR it's a separate rake task. It's faster than than a full
Jenkins spec matrix, and platform-independent, so we could possibly run it
in parallel with specs.
> 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.
>
I agree. I'm +1 with jettisoning and/or entirely.
I'm also +1 on adding Lint/AssignmentInCondition, both because I've seen
the occasional bug that would be caught by this, and more often I've seen
some confusion for code readers that would be caught by this ("did they
really mean an assignment here?"). I believe it can be addressed simply by
parenthesizing the expression.
That said, I recognize that assignment-in-conditions is widely used and
thus this one may be controversial, so I'm curious to hear others' thoughts.
>
>> 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.
>
Why have a separate tool? I'd rather have fewer tools if it gets the job
done, but I may be missing your point. That said, I'd be inclined to go
light on pure formatting cops unless they catch bugs or really help with
readability. Are the following uncontroversial?
Whitespace cleanup:
Style/TrailingBlankLine
Style/TrailingWhitespace
Style/IndentationConsistency
Style/IndentationWidth
Those four are primarily of interest to me b/c lots of editors do these
things automagically (good thing) so some PRs end up with unrelated edits
to fix the above. Might as well nuke them once and for all and get cleaner
PRs.
Similarly, alignment checkers catch readability issues:
Lint/BlockAlignment
Lint/DefEndALignment
Lint/EndAlignment
Okay, there are more I find tempting but that's enough for now ....
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/CALsUZFE45L%3D0ECAzPY%2BW2UmP_nGYSmC6ktxb%2Bwpk2r%2B4%2BcNNvg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
