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. > > On Thu, Jul 17, 2014 at 10:12 AM, Kylo Ginsberg <k...@puppetlabs.com> > wrote: > >> 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 >> >> >>> >>> On Thu, Jul 17, 2014 at 9:23 AM, Adrien Thebo <adr...@puppetlabs.com> >>> 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 <k...@puppetlabs.com> >>> wrote: >>> >> >>> >> On Wed, Jul 16, 2014 at 10:34 AM, Rahul Gopinath < >>> ra...@puppetlabs.com> >>> >> 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 >>> >>> <erik.gustav.da...@gmail.com> 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 <ra...@puppetlabs.com> >>> 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 >>> >>> >> <erik.gustav.da...@gmail.com> 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 <ra...@puppetlabs.com> 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 >>> >>> >> >>> <r...@puppetlabs.com> >>> >>> >> >>> wrote: >>> >>> >> >>>> >>> >>> >> >>>> >>> >>> >> >>>> >>> >>> >> >>>> On Mon, Jul 14, 2014 at 12:56 PM, 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. >>> >>> >> >>>>> >>> >>> >> >>>>> 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! >>> >>> >> >>>>> >>> >>> >> >>>>> -- >>> >>> >> >>>>> 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 >>> >>> >> >>>> >>> >>> >> >>>> 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/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 puppet-dev+unsubscr...@googlegroups.com. >>> >>> >> >> 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 puppet-dev+unsubscr...@googlegroups.com. >>> >>> >> > 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 puppet-dev+unsubscr...@googlegroups.com. >>> >>> >> 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 >>> >> k...@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+unsubscr...@googlegroups.com. >>> >> 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 puppet-dev+unsubscr...@googlegroups.com. >>> > 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 puppet-dev+unsubscr...@googlegroups.com. >>> 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 >> 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/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 puppet-dev+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/puppet-dev/CALVJ9SK8UbNmu30aqwG50iLg50Pk%2BTVDezGZFKd%3DKmwFcrLCSQ%40mail.gmail.com > <https://groups.google.com/d/msgid/puppet-dev/CALVJ9SK8UbNmu30aqwG50iLg50Pk%2BTVDezGZFKd%3DKmwFcrLCSQ%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/CANhgQXthZuvDHP0EUkhXOF6vdJTvXnZR8em1J-%3DnfeedY9t20g%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
