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?
On Thu, Jul 17, 2014 at 10:12 AM, Kylo Ginsberg <[email protected]> wrote:
> On Thu, Jul 17, 2014 at 10:06 AM, Rahul Gopinath <[email protected]>
> 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
>
>
>>
>> On Thu, Jul 17, 2014 at 9:23 AM, Adrien Thebo <[email protected]>
>> wrote:
>> > 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.
>> >
>> >
>> > On Thu, Jul 17, 2014 at 7:11 AM, Kylo Ginsberg <[email protected]>
>> wrote:
>> >>
>> >> On Wed, Jul 16, 2014 at 10:34 AM, Rahul Gopinath <[email protected]
>> >
>> >> wrote:
>> >>>
>> >>> 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
>> >>> <[email protected]> wrote:
>> >>> > 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.
>> >>> >
>> >>> >
>> >>> >
>> >>> > On 16 July 2014 18:12, Rahul Gopinath <[email protected]> wrote:
>> >>> >>
>> >>> >> 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
>> >>> >>
>> >>> >> On Wed, Jul 16, 2014 at 12:21 AM, Erik Dalén
>> >>> >> <[email protected]> wrote:
>> >>> >> > 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.
>> >>> >> >
>> >>> >> >
>> >>> >> > On 15 July 2014 19:46, rahul <[email protected]> wrote:
>> >>> >> >>
>> >>> >> >> 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.
>> >>> >> >>
>> >>> >> >>
>> >>> >> >> On Monday, July 14, 2014 11:56:16 AM UTC-7, Brian LaMetterey
>> 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.
>> >>> >> >>>
>> >>> >> >>> Have we done an initial run to see how much crime we have? Is
>> it
>> >>> >> >>> a
>> >>> >> >>> daunting amount?
>> >>> >> >>>
>> >>> >> >>>
>> >>> >> >>> On Mon, Jul 14, 2014 at 11:07 AM, Rob Reynolds
>> >>> >> >>> <[email protected]>
>> >>> >> >>> wrote:
>> >>> >> >>>>
>> >>> >> >>>>
>> >>> >> >>>>
>> >>> >> >>>> On Mon, Jul 14, 2014 at 12:56 PM, Kylo Ginsberg
>> >>> >> >>>> <[email protected]>
>> >>> >> >>>> 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.
>> >>> >> >>>>>
>> >>> >> >>>>> 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
>> >>> >> >>>>> [email protected]
>> >>> >> >>>>>
>> >>> >> >>>>> 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 [email protected].
>> >>> >> >>>>>
>> >>> >> >>>>> 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
>> >>> >> >>>>
>> >>> >> >>>> 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 [email protected].
>> >>> >> >>>> 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
>> >>> >> >>> 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 [email protected].
>> >>> >> >> 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
>> >>> >> >
>> >>> >> > --
>> >>> >> > 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 [email protected].
>> >>> >> > To view this discussion on the web visit
>> >>> >> >
>> >>> >> >
>> >>> >> >
>> https://groups.google.com/d/msgid/puppet-dev/CAAAzDLfzpN1wMivHNYsg%2BwWqgd5qG7D%3D5avapBDFvN214HPNSQ%40mail.gmail.com
>> .
>> >>> >> >
>> >>> >> > 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 [email protected].
>> >>> >> 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
>> >>> >
>> >>
>> >>
>> >> 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.
>> >>
>> >> --
>> >> Kylo Ginsberg
>> >> [email protected]
>> >>
>> >> 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 [email protected].
>> >> 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
>> >
>> > --
>> > 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 [email protected].
>> > To view this discussion on the web visit
>> >
>> https://groups.google.com/d/msgid/puppet-dev/CALVJ9SJ5mQc6u9wRjFNnsURBAfbJU%3D9dc-owcNYNwkwVd8u23Q%40mail.gmail.com
>> .
>> >
>> > 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 [email protected].
>> 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.
>>
>
>
>
> --
> Kylo Ginsberg
> [email protected]
>
> *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 [email protected].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/puppet-dev/CALsUZFG_gtNQk5qAFES4Ku09nd70iTCLZ9LDgsT_ZFQLh5h%3DeQ%40mail.gmail.com
> <https://groups.google.com/d/msgid/puppet-dev/CALsUZFG_gtNQk5qAFES4Ku09nd70iTCLZ9LDgsT_ZFQLh5h%3DeQ%40mail.gmail.com?utm_medium=email&utm_source=footer>
> .
>
> For more options, visit https://groups.google.com/d/optout.
>
--
Adrien Thebo | Puppet Labs
--
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 [email protected].
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.
