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)
On Fri, Aug 8, 2014 at 3:13 PM, Andy Parker <[email protected]> wrote: > On Fri, Aug 8, 2014 at 3:11 PM, Rahul Gopinath <[email protected]> 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 :) > >> >> (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. > > > > > -- > 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/CANhgQXsF3xE8uYWDRufOEyxf6ry6nc5TuibMvujifjVENiR6%3Dg%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%2BemFfwbXed640vvSfoS50-v2xPO9NMCWu%3DcLJOyoVvX85Q%3DHg%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
