On Mon, Jul 14, 2014 at 3:08 PM, Kylo Ginsberg <k...@puppetlabs.com> wrote:
> 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.
>
Yep, I noticed that. Does houndci just work from the rubocop configs? If it
does, then I'd be a little worried that we are limiting ourselves to only
using rubocop for checking PRs. My concern might not be valid and it is all
going to be ok :)
>
>
>> 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.
>
>
Yes, AssignmentInCondition is a really tempting thing to keep around, but
it leads to really long expressions that I sometimes spend a bit of time
unravelling. Often it is just as easy to extract out an assignment and
maybe nest some ifs, if that is needed. I'd be +1 for getting rid of them.
> 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?
>
>
I'm thinking of a separate tool because code formatting is one of those
things where it is, in my opinion, just inane to have humans doing it. In a
previous life, when I worked in perl, the developers argued repeatedly
about what should line up, where spaces should be, when a { should be on
the end of a line or on the next line. Eventually we all just got together
and spent a few hours going over the Perltidy configs, wrote up a config
file, checked that into the repo, and then proclaimed that the only
accepted formatting was that created by the configuration. Most of us ended
up created save handlers in our editors to just run the file through
Perltidy every time we saved.
So if we can get a tool like that, then there isn't any need for checking
the formatting, instead you just format the code and check it in.
> 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
> <https://groups.google.com/d/msgid/puppet-dev/CALsUZFE45L%3D0ECAzPY%2BW2UmP_nGYSmC6ktxb%2Bwpk2r%2B4%2BcNNvg%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/CANhgQXvi-FXfVcNDH75wsX2DZZtN0c58P8enUHOP-nt8GpE5%3Dw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
