RFC: Static Analysis of the Puppet project

[Kylo Ginsberg]

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. 

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

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.

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

So, thoughts?

Kylo

--

Kylo Ginsberg

ky...@puppetlabs.com

Join us at PuppetConf 2014, September 20-24 in San Francisco

Register by July 31st to take advantage of the Early Bird discount —save $249!

[Rob Reynolds]

--
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+...@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.
For more options, visit https://groups.google.com/d/optout.

I think it would greatly increase the quality of contributions if the "cops" started catching things and failing the PR builds. Being picky with what we start evaluating I think is the right call and what Andy and Rahul were already working out.

--

Rob Reynolds

Developer, Puppet Labs

[Brian LaMetterey]

Keep in mind that we can always take a layered approach.  Could hire a small number of cops, then add more as our crime rate decreases.

Have we done an initial run to see how much crime we have?  Is it a daunting amount?

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CAMJiBK4ZzCG_5Noa-3ctfcmgHCArXri6wqXUnbypeQ%3DK%3Dnxz_A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Join us at PuppetConf 2014, September 22-24 in San Francisco - http://puppetconf.com 

[Andy Parker]

On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <ky...@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

ky...@puppetlabs.com

Join us at PuppetConf 2014, September 20-24 in San Francisco

Register by July 31st to take advantage of the Early Bird discount —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+...@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.
For more options, visit https://groups.google.com/d/optout.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 22-24 in San Francisco

Register by May 30th to take advantage of the Early Adopter discount —save $349!

[Kylo Ginsberg]

On Mon, Jul 14, 2014 at 11:55 AM, Brian LaMetterey <brian.la...@puppetlabs.com> wrote:

Keep in mind that we can always take a layered approach.  Could hire a small number of cops, then add more as our crime rate decreases.

Absolutely. This will be a living tool which we can tweak going forward - adding/removing/configuring cops as makes sense over time.

 

Have we done an initial run to see how much crime we have?  Is it a daunting amount?

Depends on the definition of crime :) But for the cops I mentioned, there is data in the PR and comments.

Btw, I should have said that I'm pretty confident that we wouldn't want to just enable *all* of rubocop, which includes some very opinionated style-y checks, etc. So for now I'd be especially interested in opinions on what *is* valuable.

Kylo

[Kylo Ginsberg]

So now putting on my opinionated hat ;)

On Mon, Jul 14, 2014 at 12:04 PM, Andy Parker <an...@puppetlabs.com> wrote:

On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <ky...@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

[Andy Parker]

On Mon, Jul 14, 2014 at 3:08 PM, Kylo Ginsberg <ky...@puppetlabs.com> wrote:

So now putting on my opinionated hat ;)

On Mon, Jul 14, 2014 at 12:04 PM, Andy Parker <an...@puppetlabs.com> wrote:

On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <ky...@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

ky...@puppetlabs.com

Join us at PuppetConf 2014, September 20-24 in San Francisco

Register by July 31st to take advantage of the Early Bird discount —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+...@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.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 22-24 in San Francisco

Register by May 30th to take advantage of the Early Adopter discount —save $349!

[Adrien Thebo]

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 :)

It does look like houndci is hardcoded to only use Rubocop for style checking.

 

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 22-24 in San Francisco

Register by May 30th to take advantage of the Early Adopter discount —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+...@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.

--

Adrien Thebo | Puppet Labs

[Henrik Lindberg]

On 2014-15-07 24:08, Kylo Ginsberg wrote:
> So now putting on my opinionated hat ;)
>
> On Mon, Jul 14, 2014 at 12:04 PM, Andy Parker <an...@puppetlabs.com
> <mailto:an...@puppetlabs.com>> wrote:
>
> On Mon, Jul 14, 2014 at 10:56 AM, Kylo Ginsberg <ky...@puppetlabs.com
> <mailto:ky...@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.
>

And we have merges...

+1, not needed.

> 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.
>

I am fine with parenthesizing, will not catch every such bug anyway
since it is common to have parentheses and && || etc. It is no
replacement for review. May cause mork work fixing what we currently
have than what it gives in return. Don't mind though, and I will always
parenthesize my conditional assignments from now on (anyway).

> 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

ok

> Style/TrailingWhitespace
ok

> Style/IndentationConsistency
If this means no mixed spaces and tabs

> Style/IndentationWidth
This can mean several things, such as how much something should be
indented - in general it should follow the rules, but there are
exceptions esp. when the same pattern is repeated and formatting makes
it harder to see that pattern (more mental capacity is required) - and
then people start fighting with the formatter and adding *no format*
here there and everywhere and soon we have to forbid *no format*.

>
> 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 ....
>

I think the basic hygiene checks (trailing spaces, no line break last,
etc.) can be enforced but that formatting is something best done by the
authoring tool / the author. Sure have a style checker / linter - but as
an aid, not a prescription.

- henrik
--

Visit my Blog "Puppet on the Edge"
http://puppet-on-the-edge.blogspot.se/

[rahul]

The total number of offenses on enabling all cops is 38303, of which 8769 are in lib/puppet/pops

Not all the cops may be useful, and a few of them are controversial.

[rahul]

[..snip..]

 

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.

It is possible to get back most idiomatic constructions by slightly tweaking the cops. In fact it is rather easy to do so, and here[1] is a custom cop that checks for And/Or only in conditionals.

- I feel that the boolean control flow idiom is actually useful.

A pattern that I have seen often in code is of the type

unless x = mymethod()

  x.do_something

end

Which falls foul of Lint/AssignmentInCondition

I think that it would be better written as

`x = mymethod() and x.do_something`

rather than

`(x = mymethod()) && x.do_something`

which is recommended in the ruby style guide from rubocop if we restrict and/or to purely control flow operators.

[1] https://github.com/vrthra/puppet/commit/07cced3157a957b1b49de00a34da075af580fad7

[Andy Parker]

On Tue, Jul 15, 2014 at 11:01 AM, rahul <ra...@puppetlabs.com> wrote:

[..snip..]

 

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.

It is possible to get back most idiomatic constructions by slightly tweaking the cops. In fact it is rather easy to do so, and here[1] is a custom cop that checks for And/Or only in conditionals.

- I feel that the boolean control flow idiom is actually useful.

A pattern that I have seen often in code is of the type

unless x = mymethod()

  x.do_something

end

I think you unwittingly just provided an example of exactly why I like neither assignments in expressions nor unless. What you just wrote is, "call do_something on x if x is nil or false". It is really easy to write that unintentionally with assignments and unless combined. :)

 

Which falls foul of Lint/AssignmentInCondition

I think that it would be better written as

`x = mymethod() and x.do_something`

rather than

`(x = mymethod()) && x.do_something`

which is recommended in the ruby style guide from rubocop if we restrict and/or to purely control flow operators.

This is starting to get into territory of religious wars, but I think we should be looking for what construction is the most obviously correct from the standpoint of a reader. I find that "and" isn't the most straightforward nor easily changed construction. For instance, your example could be written without "and" as:

  x = mymethod()

  if x

    x.do_something()

  end

Now, granted, that is 4 lines instead of 1, but it also makes a few things really obvious to me:

  * There isn't an else clause. Did the author forget that? Maybe it doesn't really matter? Is there some construction later on that is duplicating the equivalent of the else clause that should be refactored to be combined with this?

  * There is clearly a variable x that is being created and bound to a value

  * x is used later on

  * A decision is made based on the value of x. This one brings up the question of if the decision needed in the first place.

  * When there is another piece of information that needs to affect the call to do_something, there is a clear place to put it.

 

[1] https://github.com/vrthra/puppet/commit/07cced3157a957b1b49de00a34da075af580fad7

--
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+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/d3442dc7-2193-44aa-a000-18ecc2327082%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Henrik Lindberg]

I do not like the "and" "or"; to me code is clearer without them.

>
> x = mymethod()
> if x
> x.do_something()
> end
>
> Now, granted, that is 4 lines instead of 1, but it also makes a few
> things really obvious to me:
>

You now also have 1 assignment, and two references to x. Unfortunately
we have to care very much about manual optimization. If that was not the
case I would agree. I think use of and/or does not have any performance
advantages, they are basically conditional test/jumps.

- henrik


--

Visit my Blog "Puppet on the Edge"
http://puppet-on-the-edge.blogspot.se/

[Erik Dalén]

Don't know how many they are causing, but you should probably exclude the generated grammar.rb and egrammar.rb files. The PR should be updated to do this as well.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/77907278-9756-4ec4-a7fe-4d165a3cf9db%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
Erik Dalén

[Rahul Gopinath]

I see only *.ra|*.ry files (no grammar.rb)

| find . | grep grammar
./lib/puppet/external/nagios/grammar.ry
./lib/puppet/parser/grammar.ra
./lib/puppet/pops/parser/egrammar.ra

We are currently limiting the scanning to *.rb files

> https://groups.google.com/d/msgid/puppet-dev/CAAAzDLfzpN1wMivHNYsg%2BwWqgd5qG7D%3D5avapBDFvN214HPNSQ%40mail.gmail.com.

[Erik Dalén]

right, the generated files are:

lib/puppet/parser/parser.rb

lib/puppet/pops/parser/eparser.rb

lib/puppet/external/nagios/parser.rb

They are generated from those .ry and .ra files.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfzOtUwAp7otOUZ-oo0PcSbKSf8BRLFkGhGgu6eBUubj6A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
Erik Dalén

[Rahul Gopinath]

Thanks, I have updated the PR
https://github.com/vrthra/puppet/commit/f4f9fc4e333b2e53d63ca4b8e00d02a4f2bd47f8

On Wed, Jul 16, 2014 at 10:03 AM, Erik Dalén

> https://groups.google.com/d/msgid/puppet-dev/CAAAzDLe2mLm%3D2HjMc4r_Zo6zRFASDpcCeQB7ErvFS_U5F7hbhA%40mail.gmail.com.

[Kylo Ginsberg]

To move this forward, I propose we keep this first pass on the modest side and stick to the cops that have mostly nods above:

Lint/UnreachableCode

Lint/ConditionPosition

Lint/UselessComparison

Lint/LiteralInterpolation

Lint/ElseLayout

Style/AndOr  

This will allow us to focus on integrating rubocop into the CI pipeline (jenkins and travis/hound) and sorting out any issues there.

Once we have something in place and get a feel for how it works, we can of course have follow-on PRs for other cops discussed above like assignment-in-conditionals or formatting, etc, and those can be discussed as with any other PR.

[Adrien Thebo]

I agree, I think it's better to get this system turned on and catching issues now and we can add more cops later as the code base improves. 

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CALsUZFG5Uf3PUKnrVCH33RzaXEA2UZX0YtPzhyiWnxZKih3uwA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Adrien Thebo | Puppet Labs

[Rahul Gopinath]

For the ease of management, I would like to split that into two PR,
one dealing with the original Lint/* and a second one dealing with
Style/AndOr since the number of violations of Style/AndOr is really
large, and it may be better to tackle it separately.

> https://groups.google.com/d/msgid/puppet-dev/CALVJ9SJ5mQc6u9wRjFNnsURBAfbJU%3D9dc-owcNYNwkwVd8u23Q%40mail.gmail.com.

[Kylo Ginsberg]

On Thu, Jul 17, 2014 at 10:06 AM, Rahul Gopinath <ra...@puppetlabs.com> wrote:

For the ease of management, I would like to split that into two PR,
one dealing with the original Lint/* and a second one dealing with
Style/AndOr since the number of violations of Style/AndOr is really
large, and it may be better to tackle it separately.

+1

 

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfyOH0XPsBfU6%2BHgFOOqJ-bV1VASzKYJ8iJYyOaVUnKLFg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

[Adrien Thebo]

When reviewing pull request 2900 to add Rubocop and remove instances And/Or, I came across some rather interesting behavior with how the boolean operators interact with some keywords and methods. For the sake of clarity I'm copying my comment from the pull request to here for discussion:

I'm concerned about the complete removal of and and or as flow control operators. My understanding is that we're removing these to make the code more consistent and readable but I think that doing a wholesale replacement of flow control operators with boolean operators may set us about as far back as removing the flow control operators will move us forward.

Take the following example:

-      execute_prerun_command or return nil
+      execute_prerun_command || (return nil)

I think that the latter is less clear than the former. Moreover, removing the parentheses is an illegal expression:

[1] pry(main)> def logical_comp
[1] pry(main)*   false or return :x
[1] pry(main)* end  
=> nil
[2] pry(main)> logical_comp
=> :x
[3] pry(main)> def boolean_comp
[3] pry(main)*   false || return :x
[3] pry(main)* end  
SyntaxError: unexpected tSYMBEG, expecting keyword_end
  false || return :x
                   ^
[3] pry(main)> def boolean_comp
[3] pry(main)*   false || (return :x)
[3] pry(main)* end  
=> nil

The flow control operators have a lower precedence for a specific reason - when explicitly used for flow control they behave correctly. Their very low precedence makes them suitable for this specific use case. As we've seen using flow control operators in conditional expressions can do surprising things, which is why we certainly should strip them from conditionals. However removing the flow control operators and replacing them with boolean operators will flip around the problem - instead of precedence in conditionals screwing us up, we'll have precedence in flow control doing unexpected things. This will remove one class of errors but introduce another.

We can continue forward with removing flow control operators for the sake of consistency and unambiguity, but doing this means that we should remove the use of flow control style operations entirely. Instead of using and/or/&&/|| we should use if instead. This will clearly indicate that we're doing flow control in the code and will completely sidestep the entire problem of operator precedence. However, if we're not going to replace flow control operators with true conditionals, then I don't think we should replace flow control operators at all.

- - -

So the boolean operators don't have equivalent behavior to the flow control operators in a number of circumstances - how do we want to proceed with this?

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CALsUZFG_gtNQk5qAFES4Ku09nd70iTCLZ9LDgsT_ZFQLh5h%3DeQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Andy Parker]

On Tue, Jul 29, 2014 at 1:35 PM, Adrien Thebo <adr...@puppetlabs.com> wrote:

When reviewing pull request 2900 to add Rubocop and remove instances And/Or, I came across some rather interesting behavior with how the boolean operators interact with some keywords and methods. For the sake of clarity I'm copying my comment from the pull request to here for discussion:

I'm concerned about the complete removal of and and or as flow control operators. My understanding is that we're removing these to make the code more consistent and readable but I think that doing a wholesale replacement of flow control operators with boolean operators may set us about as far back as removing the flow control operators will move us forward.

I share some of your concerns about the changes. I am absolutely in favor of adding the static checks, and I understand that it is easier to write the transformations if the structure of the code is left relatively untouched, but it also leads to the problems that are outlined in the rest of your message. 

Take the following example:

-      execute_prerun_command or return nil
+      execute_prerun_command || (return nil)

I think that the latter is less clear than the former. Moreover, removing the parentheses is an illegal expression:

Absolutely. I haven't looked at the full context of that statement, but there is likely something that is being guarded by the return value of execute_prerun_command, but the guard isn't made clear by the structure of the code. What the transformation does is to open it up to possible problems later on. 

[snip]

So the boolean operators don't have equivalent behavior to the flow control operators in a number of circumstances - how do we want to proceed with this?

I think we can continue on the current course, *if* there is follow up to fix those very strange expressions.

 

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CALVJ9SK8UbNmu30aqwG50iLg50Pk%2BTVDezGZFKd%3DKmwFcrLCSQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 22-24 in San Francisco

Register by May 30th to take advantage of the Early Adopter discount —save $349!

[Rahul Gopinath]

Andy,

[...]

> Absolutely. I haven't looked at the full context of that statement, but
> there is likely something that is being guarded by the return value of
> execute_prerun_command, but the guard isn't made clear by the structure of
> the code. What the transformation does is to open it up to possible problems
> later on.
>
> [snip]
>>
>> So the boolean operators don't have equivalent behavior to the flow
>> control operators in a number of circumstances - how do we want to proceed
>> with this?
>>
>
> I think we can continue on the current course, *if* there is follow up to
> fix those very strange expressions.

I am not entirely sure what you mean by the strange expressions here.
The pull request does not have the kind of expressions that Adrien
talks about. He is also pointing out that _if_ parenthesis is removed
from the equivalent expression, it results in a syntax error. So there
cant be a one to one replacement without adding a wrapping
parenthesis. This is not the case in the pull request.

[Andy Parker]

By "strange expressions" I was referring to ones that are in forms that a developer would seldom, if ever, write. Things like the example that Adrien gave, where the parenthesis are required to get the code to parse correctly.

Right now the PRs are doing a mechanical transformation to remove a keyword that we don't want to use. What is missing is that it isn't transforming the code into what later changes to that code should preserve. Or put another way, if we got a PR that contained new code that looked like that we would reject it, I think. It passes the test of not using disallowed operators, but it doesn't pass the test of being written in a form that a reader would expect. Here is an example https://github.com/puppetlabs/puppet/pull/2900/files#diff-f9ef6a029a765863382f55a75a8fbd7cR27

 

--
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+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfzVKzhQv%2Bx1gZRZt-FX9jP1gx5HMUXdtRJcbGjoWgHe3w%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Jeff McCune]

On Tue, Jul 29, 2014 at 4:42 PM, Andy Parker <an...@puppetlabs.com> wrote:

Here is an example https://github.com/puppetlabs/puppet/pull/2900/files#diff-f9ef6a029a765863382f55a75a8fbd7cR27

Could you help me understand what it should look like?

-Jeff

[Andy Parker]

if @render_as

  @render_as

else

  raise ....

end

Or it could be

if !@render_as

  raise ...

end

@render_as

 

-Jeff

--
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+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CAOXx1vHSHOQN5kdU%2Bq0u8NV9PPT8z74%3DeM3M0vJ%2B172G07u2Xg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Daniele Sluijters]

Because 3 is always better than 2 SaaS services, has any consideration been given to Codeclimate? We've been using it at Nedap for a while now and so far it has helped identify some (big) issues in our code bases and helped us keep new projects on track.

Their 'security monitor' is remarkably useful at times and sometimes lights up like a Christmas tree for no reason. The 'hotspots', aka stuff you should refactor, has helped us out quite a bit at times too.

I doubt they can be used as a remplacement to Rubocop, but, layers :).

[Rahul Gopinath]

On Tue, Jul 29, 2014 at 6:08 PM, Daniele Sluijters
<daniele....@gmail.com> wrote:
> Codeclimate

I haven't actually, but it seems that the tools they use on ruby is
Flog and Flay[1]
which looks for duplication and complexity, and these I had looked at
individually.
They are useful at what they do, though as you point out, not a
replacement for rubocop
style analysis

[1] http://docs.codeclimate.com/article/160-open-source-tools-used-by-code-climate

[Kylo Ginsberg]

On Tue, Jul 29, 2014 at 4:42 PM, Andy Parker <an...@puppetlabs.com> wrote:

Right now the PRs are doing a mechanical transformation to remove a keyword that we don't want to use. What is missing is that it isn't transforming the code into what later changes to that code should preserve. Or put another way, if we got a PR that contained new code that looked like that we would reject it, I think. It passes the test of not using disallowed operators, but it doesn't pass the test of being written in a form that a reader would expect. 

I agree that the purely mechanical transformation applied to the genuinely flow control cases introduces constructs that would slow me down as a code reader (and that I'd be very unlikely to write).

So are there objections to converting such cases to use 'if', etc? Personally I'd find that clearer and easier to read. And it would still allow us to eliminate the and/or keywords which we've identified as the source of some bugs/confusion.

Kylo

[Kylo Ginsberg]

On Tue, Jul 29, 2014 at 6:08 PM, Daniele Sluijters <daniele....@gmail.com> wrote:

I imagine this audience has seen this before, but if not:

https://codeclimate.com/github/puppetlabs/puppet/

I haven't played with Code Climate in earnest, but I have peeked at the above sometimes out of curiosity.

Kylo

--

Kylo Ginsberg

ky...@puppetlabs.com

Join us at PuppetConf 2014, September 20-24 in San Francisco

Register by July 31st to take advantage of the Early Bird discount —save $249!

[rahul]

So to summarize, this is our plan for Rubocop:

- We propose to enable AndOr cop in small chunks, giving preference to those files/directories that are heavily in development.

- For AndOr, the conclusion seems to be to avoid keywords completely, and ensure that the instances where they are used are changed do not hurt readability.

- As a prototype, we have turned on AndOr on lib/pops directory PR 2892

[Kylo Ginsberg]

On Tue, Aug 5, 2014 at 3:50 PM, rahul <ra...@puppetlabs.com> wrote:

So to summarize, this is our plan for Rubocop:

- We propose to enable AndOr cop in small chunks, giving preference to those files/directories that are heavily in development.

- For AndOr, the conclusion seems to be to avoid keywords completely, and ensure that the instances where they are used are changed do not hurt readability.

- As a prototype, we have turned on AndOr on lib/pops directory PR 2892

Also a heads-up that for pull requests:

1) a week or so ago, we added a travis job that fails if any of the .rubocop.yml enabled cops report anything (these are just the cops that were uncontroversial at the beginning of this thread)

2) just now, I turned on houndci.com which will comment on pull requests based on the same configuration 

Note that hound *can* be configured with a separate config file of its own, but we don't have one, so it falls back to the .rubocop.yml. If we wanted to have a set of cops which triggered comments on the PRs, but didn't figure travis fails, we could get that by having a separate houndci.yml. Not sure what I think of that, but just putting the idea out there.

Kylo

 

On Tuesday, July 29, 2014 11:00:46 PM UTC-7, Kylo Ginsberg wrote:

On Tue, Jul 29, 2014 at 4:42 PM, Andy Parker <an...@puppetlabs.com> wrote:

Right now the PRs are doing a mechanical transformation to remove a keyword that we don't want to use. What is missing is that it isn't transforming the code into what later changes to that code should preserve. Or put another way, if we got a PR that contained new code that looked like that we would reject it, I think. It passes the test of not using disallowed operators, but it doesn't pass the test of being written in a form that a reader would expect. 

I agree that the purely mechanical transformation applied to the genuinely flow control cases introduces constructs that would slow me down as a code reader (and that I'd be very unlikely to write).

So are there objections to converting such cases to use 'if', etc? Personally I'd find that clearer and easier to read. And it would still allow us to eliminate the and/or keywords which we've identified as the source of some bugs/confusion.

Kylo

--

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+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/ab872dad-c258-4e09-81b3-8c13f17bc968%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Kylo Ginsberg

ky...@puppetlabs.com

Join us at PuppetConf 2014, September 20-24 in San Francisco

Register by September 8th to take advantage of the Final Countdown —save $149!

[Kylo Ginsberg]

On Wed, Aug 6, 2014 at 5:39 PM, Kylo Ginsberg <ky...@puppetlabs.com> wrote:

On Tue, Aug 5, 2014 at 3:50 PM, rahul <ra...@puppetlabs.com> wrote:

So to summarize, this is our plan for Rubocop:

- We propose to enable AndOr cop in small chunks, giving preference to those files/directories that are heavily in development.

- For AndOr, the conclusion seems to be to avoid keywords completely, and ensure that the instances where they are used are changed do not hurt readability.

- As a prototype, we have turned on AndOr on lib/pops directory PR 2892

Also a heads-up that for pull requests:

1) a week or so ago, we added a travis job that fails if any of the .rubocop.yml enabled cops report anything (these are just the cops that were uncontroversial at the beginning of this thread)

2) just now, I turned on houndci.com which will comment on pull requests based on the same configuration 

Note that hound *can* be configured with a separate config file of its own, but we don't have one, so it falls back to the .rubocop.yml. If we wanted to have a set of cops which triggered comments on the PRs, but didn't figure travis fails, we could get that by having a separate houndci.yml. Not sure what I think of that, but just putting the idea out there.

Actually houndci doesn't seem to be respecting our .rubocop.yml so I turned it off for now.

Kylo

 

Kylo

 

On Tuesday, July 29, 2014 11:00:46 PM UTC-7, Kylo Ginsberg wrote:

On Tue, Jul 29, 2014 at 4:42 PM, Andy Parker <an...@puppetlabs.com> wrote:

Right now the PRs are doing a mechanical transformation to remove a keyword that we don't want to use. What is missing is that it isn't transforming the code into what later changes to that code should preserve. Or put another way, if we got a PR that contained new code that looked like that we would reject it, I think. It passes the test of not using disallowed operators, but it doesn't pass the test of being written in a form that a reader would expect. 

I agree that the purely mechanical transformation applied to the genuinely flow control cases introduces constructs that would slow me down as a code reader (and that I'd be very unlikely to write).

So are there objections to converting such cases to use 'if', etc? Personally I'd find that clearer and easier to read. And it would still allow us to eliminate the and/or keywords which we've identified as the source of some bugs/confusion.

Kylo

--

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+...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/ab872dad-c258-4e09-81b3-8c13f17bc968%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Kylo Ginsberg

ky...@puppetlabs.com

Join us at PuppetConf 2014, September 20-24 in San Francisco

Register by September 8th to take advantage of the Final Countdown —save $149!

[Rahul Gopinath]

While hacking rubocop, I found that I can get it to autocorrect even
more `Style/AndOr` violations if I replace the use of the `not`
keyword with `!` first. The Rubocop is able to do the necessary
changes automatically. So I think we should turn on the `Style/Not`
cop also for our code for three reasons
1. It is essentially free, with rubocop able to supply the entire set
of corrections in its autocorrect mode
2. It is consistent without goal of avoiding keywords with confusing precedence.
3. Rubocop autocorrect is the best we can hope for, since it ensures
that the AST resulting from the changes are same, and hence the
semantics of the new and old fragments are same. Hence we avoid bugs
that cold go undetected otherwise.
I also propose to turn on the `Style/SpaceAfterNot` which disallows
space after the operator `!` so that usage such as `! mymethod` is
flagged in favor or `!mymethod`

Are there any objections to both these?

If you would like to see what changes these two require, see my PR at
https://github.com/puppetlabs/puppet/pull/2944

> https://groups.google.com/d/msgid/puppet-dev/CALsUZFF4NF%3D6hoA3DUb6NturZ1KHLR2Y4bNiwoYiVrVse3fzgg%40mail.gmail.com.

[Andy Parker]

On Thu, Aug 7, 2014 at 6:09 PM, Rahul Gopinath <ra...@puppetlabs.com> wrote:

While hacking rubocop, I found that I can get it to autocorrect even
more `Style/AndOr` violations if I replace the use of the `not`
keyword with `!` first. The Rubocop is able to do the necessary
changes automatically. So I think we should turn on the `Style/Not`
cop also for our code for three reasons
1. It is essentially free, with rubocop able to supply the entire set
of corrections in its autocorrect mode
2. It is consistent without goal of avoiding keywords with confusing precedence.

I actually thought that not would have gone hand in hand with and/or. Doesn't it have the same precedence problems as and/or?

Either way, I'm a +1 for adding in this rule too.

 

3. Rubocop autocorrect is the best we can hope for, since it ensures
that the AST resulting from the changes are same, and hence the
semantics of the new and old fragments are same. Hence we avoid bugs
that cold go undetected otherwise.
I also propose to turn on the `Style/SpaceAfterNot` which disallows
space after the operator `!` so that usage such as `! mymethod` is
flagged in favor or `!mymethod`

No space after ! is my preferred style. And I'll just stop any style wars and say it is the preferred style of the codebase (I am channelling the puppet code gods).

 

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfz56%3DNTedoRVbM32FVCZpnKov7BoSwewTpJR9rc%2BR2QRA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

[Rahul Gopinath]

While correcting AndOr, I come across methods calls such as `if
value.is_a? FixNum or value.is_a? Integer`

How should we parenthesize it? is it `(value.is_a? FixNum) ||
(value.is_a? Integer)` or should it be `value.is_a?(FixNum) ||
value.is_a?(Integer)` ?

(The question is specific to boolean predicates since they make up
almost all of the method calls that interact with `and` and `or`)

Is there a preference here?

> https://groups.google.com/d/msgid/puppet-dev/CANhgQXtd%3DZ0Qr9eqJOopqYFP4ER_xnCAcgKomm8wPNWy42zmsQ%40mail.gmail.com.

[Andy Parker]

On Fri, Aug 8, 2014 at 3:11 PM, Rahul Gopinath <ra...@puppetlabs.com> wrote:

While correcting AndOr, I come across methods calls such as  `if
value.is_a? FixNum or value.is_a? Integer`

How should we parenthesize it? is it `(value.is_a? FixNum) ||
(value.is_a? Integer)`  or should it be `value.is_a?(FixNum) ||
value.is_a?(Integer)` ?

value.is_a?(FixNum)

Parens around the method arguments are the clearest fix and we should put them in everywhere IMO :)

 

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfwFLqgiNaK_mSXvxWepXmJeD%2BLx2mVcKmOV_fu%3DJRqx5g%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Rahul Gopinath]

Would it make sense to add such a cop also? -- i.e A cop that bans
method calls (with at least one argument) without parenthesis
(We don't have one right now, but I think it can be done easily)

> https://groups.google.com/d/msgid/puppet-dev/CANhgQXsF3xE8uYWDRufOEyxf6ry6nc5TuibMvujifjVENiR6%3Dg%40mail.gmail.com.

[Henrik Lindberg]

On 2014-09-08 24:13, Andy Parker wrote:
> On Fri, Aug 8, 2014 at 3:11 PM, Rahul Gopinath <ra...@puppetlabs.com
> <mailto:ra...@puppetlabs.com>> wrote:
>
> While correcting AndOr, I come across methods calls such as `if
> value.is_a? FixNum or value.is_a? Integer`
>
> How should we parenthesize it? is it `(value.is_a? FixNum) ||
> (value.is_a? Integer)` or should it be `value.is_a?(FixNum) ||
> value.is_a?(Integer)` ?
>
>
> value.is_a?(FixNum)
>
> Parens around the method arguments are the clearest fix and we should
> put them in everywhere IMO :)
>

+1, except in internal DSL like logic where it reduces readability.

+1


- henrik
--

Visit my Blog "Puppet on the Edge"
http://puppet-on-the-edge.blogspot.se/

[Kylo Ginsberg]

On Sat, Aug 9, 2014 at 9:45 AM, Henrik Lindberg <henrik....@cloudsmith.com> wrote:

On 2014-09-08 24:13, Andy Parker wrote:

On Fri, Aug 8, 2014 at 3:11 PM, Rahul Gopinath <ra...@puppetlabs.com

<mailto:ra...@puppetlabs.com>> wrote:

    While correcting AndOr, I come across methods calls such as  `if
    value.is_a? FixNum or value.is_a? Integer`

    How should we parenthesize it? is it `(value.is_a? FixNum) ||
    (value.is_a? Integer)`  or should it be `value.is_a?(FixNum) ||
    value.is_a?(Integer)` ?


value.is_a?(FixNum)

Parens around the method arguments are the clearest fix and we should
put them in everywhere IMO :)

+1, except in internal DSL like logic where it reduces readability 

I definitely agree on the parentheses example above.

Wrt requiring parens around method arguments *everywhere*, I have to admit I've wanted that at times when reading puppet code, but I also suspect it would be very high touch and perhaps controversial. 

And it sounds like we don't have consensus on that one. Henrik, can you give an example or two where it would reduce readability? That might help guide the discussion a bit.

     >> So I think we should turn on the `Style/Not` cop

     >

     > Either way, I'm a +1 for adding in this rule too.
     >

     > No space after ! is my preferred style. And I'll just stop any
    style wars
     > and say it is the preferred style of the codebase (I am
    channelling the
     > puppet code gods).
     >

+1

Sounds like we have agreement on Style/Not and Style/SpaceAfterNot. Shout now if you disagree!

Kylo

[Rahul Gopinath]

Here is a related issue,

I notice that there are different conventions for spacing followed
when using method calls with parenthesis,
including `method(a,b)`, `method( a, b )` and `method( a,b )`. Does
the list have a preference for one of the styles when I add
parenthesis (to those methods that are not DSL like)?

I think our choices are
`mymethod(a,b)` -- currently supported by rubocop
`mymethod( a, b )` -- another common style

or any other...

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/puppet-dev/CALsUZFF2nFyOpvTB5Ap1mEvZ_NaC2y-VN00Pic_ZdKt%2B0_rh0g%40mail.gmail.com.

[Henrik Lindberg]

On 2014-11-08 2:01, Kylo Ginsberg wrote:
> On Sat, Aug 9, 2014 at 9:45 AM, Henrik Lindberg

> <henrik....@cloudsmith.com <mailto:henrik....@cloudsmith.com>>

> wrote:
>
> On 2014-09-08 24:13, Andy Parker wrote:
>
> On Fri, Aug 8, 2014 at 3:11 PM, Rahul Gopinath
> <ra...@puppetlabs.com <mailto:ra...@puppetlabs.com>

> <mailto:ra...@puppetlabs.com <mailto:ra...@puppetlabs.com>>> wrote:
>
> While correcting AndOr, I come across methods calls such as
> `if
> value.is_a? FixNum or value.is_a? Integer`
>
> How should we parenthesize it? is it `(value.is_a? FixNum) ||
> (value.is_a? Integer)` or should it be `value.is_a?(FixNum) ||
> value.is_a?(Integer)` ?
>
>
> value.is_a?(FixNum)
>
> Parens around the method arguments are the clearest fix and we
> should
> put them in everywhere IMO :)
>
>
> +1, except in internal DSL like logic where it reduces readability
>
>
> I definitely agree on the parentheses example above.
>
> Wrt requiring parens around method arguments *everywhere*, I have to
> admit I've wanted that at times when reading puppet code, but I also
> suspect it would be very high touch and perhaps controversial.
>
> And it sounds like we don't have consensus on that one. Henrik, can you
> give an example or two where it would reduce readability? That might
> help guide the discussion a bit.
>
>

The first that comes to mind is spec tests - it feels like you are using
a different language than ruby, but it is really a bunch of method calls
without parentheses.

i.e. do you want to write it like this?

context("this context") do
it("the thingy can ...") do
...
end
end

IMO, that adds no value at all.

In models:

class Program < PopsObject
contains_one_uni 'body', Expression
has_many 'definitions', Definition
has_attr 'source_text', String
has_attr 'source_ref', String
has_many_attr 'line_offsets', Integer
has_attr 'locator', Object
end

In the new function API:

dispatch :handle_enumerable do
param 'Any', :enumerable
param 'Hash', :options
end

Enforcing () in places like these sort of goes against the idea of using
an internal DSL.

[Joshua Partlow]

On Sunday, August 10, 2014, Rahul Gopinath <ra...@puppetlabs.com> wrote:
> Here is a related issue,
>
> I notice that there are different conventions for spacing followed
> when using method calls with parenthesis,
> including `method(a,b)`, `method( a, b )` and `method( a,b )`. Does
> the list have a preference for one of the styles when I add
> parenthesis (to those methods that are not DSL like)?
>
> I think our choices are
> `mymethod(a,b)`   -- currently supported by rubocop
> `mymethod( a, b )`  -- another common style
>
> or any other...

I guess I'm in the any other camp.

 mymethod(a, b)


--

Josh Partlow

jpar...@puppetlabs.com

Developer, Puppet Labs

[Thomas Hallgren]

On 2014-08-11 17:06, Joshua Partlow wrote:

On Sunday, August 10, 2014, Rahul Gopinath <ra...@puppetlabs.com> wrote:
> Here is a related issue,
>
> I notice that there are different conventions for spacing followed
> when using method calls with parenthesis,
> including `method(a,b)`, `method( a, b )` and `method( a,b )`. Does
> the list have a preference for one of the styles when I add
> parenthesis (to those methods that are not DSL like)?
>
> I think our choices are
> `mymethod(a,b)`   -- currently supported by rubocop
> `mymethod( a, b )`  -- another common style
>
> or any other...

I guess I'm in the any other camp.

 mymethod(a, b)

That would be my camp too.

- thomas

[Kylo Ginsberg]

On Mon, Aug 11, 2014 at 8:06 AM, Joshua Partlow <joshua....@puppetlabs.com> wrote:

On Sunday, August 10, 2014, Rahul Gopinath <ra...@puppetlabs.com> wrote:
> Here is a related issue,
>
> I notice that there are different conventions for spacing followed
> when using method calls with parenthesis,
> including `method(a,b)`, `method( a, b )` and `method( a,b )`. Does
> the list have a preference for one of the styles when I add
> parenthesis (to those methods that are not DSL like)?
>
> I think our choices are
> `mymethod(a,b)`   -- currently supported by rubocop
> `mymethod( a, b )`  -- another common style
>
> or any other...

I guess I'm in the any other camp.

 mymethod(a, b)

Me too; that's the style I always use.

I find the no-spaces style too cramped and hard to read. I can read the spaces-inside-parens style; I just never write that.

Kylo

[Rahul Gopinath]

I agree with leaving () out for the DSL usage.

Are there any such instances of DSL usage within conditionals
(if/unless/while...)? If not, would you be in favor if we restrict the
enforcement to conditionals only?

> --
> 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+...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/puppet-dev/lsaiai%24f9r%241%40ger.gmane.org.

[Henrik Lindberg]

On 2014-11-08 17:44, Rahul Gopinath wrote:
> I agree with leaving () out for the DSL usage.
>
> Are there any such instances of DSL usage within conditionals
> (if/unless/while...)? If not, would you be in favor if we restrict the
> enforcement to conditionals only?
>
>

I think that would be rare (if at all in our code base at least).

- henrik

[Rahul Gopinath]

Thanks,
So just to confirm, we can try to enforce method call parenthesis
in conditionals (Does any one else have any objections?)

On a related note, As Adrien noted, there are a few legitimate use
cases for `and` and `or` keywords, where they function as control flow
operators, and help readability: e.g `cmd x or return nil` or `do_x or
raise 'error'`, which can be replaced with if/unless but I wonder how
much it will gain us in terms of readability. I think it would be nice
to leave them alone. Any thoughts in this regard?

On Mon, Aug 11, 2014 at 8:50 AM, Henrik Lindberg

> https://groups.google.com/d/msgid/puppet-dev/lsaoop%24107%241%40ger.gmane.org.

[Trevor Vaughan]

I prefer:

cmd x or return nil

as opposed to, if x then return nil, just because it flows better.

Technically, though, a standard should be standard even if it's a bit irritating at times and I think that this one might qualify.

Trevor

To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfzcMrOytP9zEy%3DS-rSET_12g0NDZwHmVULcZsK6S5n9-A%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
Trevor Vaughan
Vice President, Onyx Point, Inc
(410) 541-6699
tvau...@onyxpoint.com

-- This account not approved for unencrypted proprietary information --

[Henrik Lindberg]

On 2014-11-08 19:06, Trevor Vaughan wrote:
> I prefer:
>
> cmd x or return nil
>
> as opposed to, if x then return nil, just because it flows better.
>
> Technically, though, a standard should be standard even if it's a bit
> irritating at times and I think that this one might qualify.
>
> Trevor
>
>

It depends very much how the methods are phrased and what they do.
And there are several ways to to do that...

return nil unless it_is_ok?
return nil if bad_things_happened?

Personally I avoid designing with use of "and"/"or" and would not mind a
bit if they were not allowed anywhere. But I recognize it could be very
irritating for someone that is used to phrasing/organizing code a
different way.

- henrik

> On Mon, Aug 11, 2014 at 1:01 PM, Rahul Gopinath <ra...@puppetlabs.com
> <mailto:ra...@puppetlabs.com>> wrote:
>
> Thanks,
> So just to confirm, we can try to enforce method call parenthesis
> in conditionals (Does any one else have any objections?)
>
> On a related note, As Adrien noted, there are a few legitimate use
> cases for `and` and `or` keywords, where they function as control flow
> operators, and help readability: e.g `cmd x or return nil` or `do_x or
> raise 'error'`, which can be replaced with if/unless but I wonder how
> much it will gain us in terms of readability. I think it would be nice
> to leave them alone. Any thoughts in this regard?
>
> On Mon, Aug 11, 2014 at 8:50 AM, Henrik Lindberg
> <henrik....@cloudsmith.com

> <mailto:henrik....@cloudsmith.com>> wrote:
> > On 2014-11-08 17:44, Rahul Gopinath wrote:
> >>
> >> I agree with leaving () out for the DSL usage.
> >>
> >> Are there any such instances of DSL usage within conditionals
> >> (if/unless/while...)? If not, would you be in favor if we
> restrict the
> >> enforcement to conditionals only?
> >>
> >>
> > I think that would be rare (if at all in our code base at least).
> >
> > - henrik
> >
> >
> >>
> >> On Mon, Aug 11, 2014 at 7:00 AM, Henrik Lindberg
> >> <henrik....@cloudsmith.com

> <mailto:henrik....@cloudsmith.com>> wrote:
> >>>
> >>> On 2014-11-08 2:01, Kylo Ginsberg wrote:
> >>>>
> >>>>
> >>>> On Sat, Aug 9, 2014 at 9:45 AM, Henrik Lindberg
> >>>> <henrik....@cloudsmith.com
> <mailto:henrik....@cloudsmith.com>

> <mailto:henrik....@cloudsmith.com

> >>>> And it sounds like we don't have consensus on that one..

> <mailto:puppet-dev%2Bunsu...@googlegroups.com>.

> >>> To view this discussion on the web visit
> >>>
> >>>
> https://groups.google.com/d/msgid/puppet-dev/lsaiai%24f9r%241%40ger.gmane.org.
> >>>
> >>> For more options, visit https://groups.google.com/d/optout.
> >>
> >>
> >
> >
> > --
> >
> > Visit my Blog "Puppet on the Edge"
> > http://puppet-on-the-edge.blogspot.se/
> >
> > --
> > 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+...@googlegroups.com

> <mailto:puppet-dev%2Bunsu...@googlegroups.com>.

> > To view this discussion on the web visit
> >
> https://groups.google.com/d/msgid/puppet-dev/lsaoop%24107%241%40ger.gmane.org.
> >
> > For more options, visit https://groups.google.com/d/optout.
>
> --
> 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+...@googlegroups.com

> <mailto:puppet-dev%2Bunsu...@googlegroups.com>.

> To view this discussion on the web visit
> https://groups.google.com/d/msgid/puppet-dev/CA%2BemFfzcMrOytP9zEy%3DS-rSET_12g0NDZwHmVULcZsK6S5n9-A%40mail.gmail.com.

> For more options, visit https://groups.google..com/d/optout
> <https://groups.google.com/d/optout>.

>
>
>
>
> --
> Trevor Vaughan
> Vice President, Onyx Point, Inc
> (410) 541-6699

> tvau...@onyxpoint.com <mailto:tvau...@onyxpoint.com>

>
> -- This account not approved for unencrypted proprietary information --
>

> --
> 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+...@googlegroups.com

> <mailto:puppet-dev+...@googlegroups.com>.

> To view this discussion on the web visit

> https://groups.google.com/d/msgid/puppet-dev/CANs%2BFoUFnkiukUm6xQm6jSjFzjmGULgK2JOQ-UgPtqJ9v8GCcQ%40mail.gmail.com
> <https://groups.google.com/d/msgid/puppet-dev/CANs%2BFoUFnkiukUm6xQm6jSjFzjmGULgK2JOQ-UgPtqJ9v8GCcQ%40mail.gmail.com?utm_medium=email&utm_source=footer>.

[Kylo Ginsberg]

On Mon, Aug 11, 2014 at 11:23 AM, Henrik Lindberg <henrik....@cloudsmith.com> wrote:

On 2014-11-08 19:06, Trevor Vaughan wrote:

I prefer:

cmd x or return nil

as opposed to, if x then return nil, just because it flows better.

Technically, though, a standard should be standard even if it's a bit
irritating at times and I think that this one might qualify.

Trevor

It depends very much how the methods are phrased and what they do.
And there are several ways to to do that...

return nil unless it_is_ok?
return nil if bad_things_happened?

Personally I avoid designing with use of "and"/"or" and would not mind a bit if they were not allowed anywhere. But I recognize it could be very irritating for someone that is used to phrasing/organizing code a different way.

My vote is to drop "and"/"or" entirely. I also just avoid using them anyway, and in general, I agree with Trevor's notion above that I can live with a little bit of irritation to have a simple, universally applied standard.

Kylo