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? On Fri, Aug 8, 2014 at 9:56 AM, Andy Parker <[email protected]> wrote: > On Thu, Aug 7, 2014 at 6:09 PM, Rahul Gopinath <[email protected]> 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). > >> >> 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 >> >> >> On Thu, Aug 7, 2014 at 9:39 AM, Kylo Ginsberg <[email protected]> wrote: >> > On Wed, Aug 6, 2014 at 5:39 PM, Kylo Ginsberg <[email protected]> >> > wrote: >> >> >> >> On Tue, Aug 5, 2014 at 3:50 PM, rahul <[email protected]> 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 <[email protected]> >> >>>> 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 [email protected]. >> >>> 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 >> >> [email protected] >> >> >> >> 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 >> > [email protected] >> > >> > Join us at PuppetConf 2014, September 20-24 in San Francisco >> > Register by September 8th to take advantage of the Final Countdown —save >> > $149! >> > >> > -- >> > 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/CALsUZFF4NF%3D6hoA3DUb6NturZ1KHLR2Y4bNiwoYiVrVse3fzgg%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%2BemFfz56%3DNTedoRVbM32FVCZpnKov7BoSwewTpJR9rc%2BR2QRA%40mail.gmail.com. >> >> For more options, visit https://groups.google.com/d/optout. > > > > > -- > Andrew Parker > [email protected] > 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 [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/puppet-dev/CANhgQXtd%3DZ0Qr9eqJOopqYFP4ER_xnCAcgKomm8wPNWy42zmsQ%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%2BemFfwFLqgiNaK_mSXvxWepXmJeD%2BLx2mVcKmOV_fu%3DJRqx5g%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
