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