I was thinking about responding to the items in your email but I feel like I’ve put enough time into this discussion already. Just some general thoughts.
I think you’re mischaracterizing my argument. I am not arguing that we enable all cops. In fact, in the dynflow case I tried to open a PR with just white spacing checking enabled but closed it out after it got out of sync and had no activity: https://github.com/Dynflow/dynflow/pull/169 To your point about disabling rubocop per instance, I’m sure that reduces readability. I guess that’s subjective. And we did have discussions on foreman-dev about which cops to enable/disable. We even had a poll. The resulting PR reached an impasse though even though we had reached a consensus on foreman-dev. David On Wed, Aug 24, 2016 at 7:41 AM, Ivan Necas <[email protected]> wrote: > On Wed, Aug 24, 2016 at 10:49 AM, Marek Hulán <[email protected]> wrote: > > Few more comments below > > > >> What do you mean by "Foreman dev"? > > > > You, me, everyone who contributed to Foreman. > > > >> I’ve contributed to both foreman and > >> rubocop several times. Having worked on rubocop with all its cops > enabled > >> hasn’t been a deterrent to me even though I don’t necessarily agree with > >> all cops. > > > > Seems rubocop devs are happy with more cops than at least one foreman > dev is > > (me). That make sense, otherwise they would not contribute to rubocop I > guess > > :-) > > > >> I’m not disagreeing with you. But I still have nightmares about the > dynflow > >> source code which had no rubocop checking at all and featured a number > of > >> obscure ruby syntaxes that rubocop would have forbid. > > I'm sorry, but this seem like a straw man argument to me. I agree with > what Marek said. > The think with the obscure syntax a little to do with Rubocop, > but rather using the Algebrick gem. We gave it a try, it didn't work, > we will remove eventually. > Nothing to do with Rubocop. I'm not saying Dynflow would not benefit > from Rubocop. > And once more prio things get sorted out, we will finish that. I would > not enable all cops > though. > > I think we got quite far from the Hash rocket topic, just my last > comment on Rubocop > in general. What I see here is people saying "the more Rubocop" = "the > more consistency" > = "the more readable code". I don't agree with this reasoning. Yes, > some cops increases > the consistency, but it can just lead to your code to be consistently > unreadable. And some cops > have nothing to do with consistency. > > Let's take few examples from the Rubocop source code itself: > > https://github.com/bbatsov/rubocop/blob/v0.42.0/lib/ > rubocop/config.rb#L243 > - multi-line modifier statement - I hope nobody here would argue > for this being more readable > then standard if > > https://github.com/bbatsov/rubocop/blob/v0.42.0/lib/ > rubocop/config_loader.rb#L74 > - combination of modifier and non-modifier if: if you tried to put > everything into non-modifier form, > it would tell you that you should use the modifier one (I > tried:), which would eventually lead you > to the same case as previous one > > https://github.com/bbatsov/rubocop/blob/v0.42.0/lib/rubocop/config.rb > - IMO the 80 lines limit just makes too many lines spread across > more lines, making it harder to read; > ok, I know some people us vim in split on lower resolution, but > I don't think others and the readability > should suffer from that: I would take the max line size from > what actually helps the readability, not > what resolution somebody has, otherwise, I would just argue > that we need max 50 lines, to make it > easier for me to review the code on my phone. > > https://github.com/bbatsov/rubocop/blob/v0.42.0/lib/rubocop/cli.rb#L5 > - the comment says "The CLI is a class responsible of handling > all the command line interface logic" > - while it was true when the class was added few years ago, it's > no more valid that it's responsible > for handling *all* of the logic, because the option parser is > in separate class now. > - I would say the comment was just useless at the first place, > and got obsolete (as comments > use to) and the only reason people add it there is the policy. > - also, from personal experience with Rubocop documentation, I > would not say it helped anyhow, > I feel the pain every time I try to find what some cop expects > or how to disable that, I end up > looking at rubocop tests anyway usually. > > What I think is happening is people relying on Rubocop keeping their > code readable > and not paying any attention to it. And some cops make it also harder > to improve the readability, > (I know you can always disable the the cop per line, but I would say > that already affects > the readability in a bad way). > > I know that it might be frustrating for someone to discuss the styling > between > the developers, rather that aligning to whatever few people agree on > Rubocop > PR to merge. I asked several times to have proper discussion on the > cops to enable/disable, > rather than doing that in PRs directly. It didn't happened, which is > why we are arguing about > it every single time somebody wants to introduce some new one. > > -- Ivan > > > > > If dynflow was foreman core project and was reviewed in that way from > the first > > commit, this would not happen. Rubocop would not help too much. I tried > to run > > rubocop on dynflow. From 1346 offenses I found only 22 regarding the > obscure > > syntax, rest is "redundant return", "1.9 hash syntax", "top level class > > documentation missing" stuff. If you want to reproduce, see [1]. > > > > This thread started around has rocket syntax. So to remain on topic, I > don't > > think that the fact there is a cop for enforcing one hash syntax, it's > widely > > accepted among Ruby devs. Speaking of hash rockets, an example might be > github > > style guide - https://github.com/styleguide/ruby/hashes > > > > [1] https://gist.github.com/ares/ff6e8b19361148e2be17290f1167c7c4 > > > > -- > > Marek > > > >> I think there’s probably a happy medium between not using rubocop and > >> enabling all rubocop cops. > >> > >> > >> David > >> > >> On Tue, Aug 23, 2016 at 11:22 AM, Marek Hulán <[email protected]> > wrote: > >> > On Tuesday 23 of August 2016 07:47:31 David Davis wrote: > >> > > The plugins may not have as many contributors as foreman core but > >> > > rubocop > >> > > has more contributors (279 vs 200 for foreman) and they have pretty > much > >> > > all cops enabled. It’s only been around for 3 years (vs 7 years for > >> > > foreman) so it doesn’t seem to be a deterrent to community > >> > > contributions. > >> > > >> > I don't think this is fair comparison. Do you think that 279 > contributors > >> > of > >> > one project represent all Ruby devs or all Foreman devs? At least not > one > >> > Foreman dev, that's for sure. I don't say we should disable rubocop > but I > >> > don't think all devs are happy with all cops. Adding cops like hash > rocket > >> > makes it only worse. > >> > > >> > -- > >> > Marek > >> > > >> > > I’d probably wager that most experienced Ruby developers are aware > of > >> > > rubocop and the ruby community style guide [1]. For unexperienced > Ruby > >> > > developers, I think they are a great way to learn good Ruby coding > >> > > standards. > >> > > > >> > > That said, I agree it would be nice to get opinions of community > members > >> > > who contribute to foreman and their experiences with rubocop. > >> > > > >> > > [1] https://github.com/bbatsov/ruby-style-guide > >> > > > >> > > > >> > > David > >> > > > >> > > On Tue, Aug 23, 2016 at 3:48 AM, Marek Hulán <[email protected]> > wrote: > >> > > > I'm with Lukas, we already enforce too many things without clear > >> > > >> > benefit. > >> > > >> > > > Let's > >> > > > keep both ways accepted which is default for all rubyists. > >> > > > > >> > > > > > I think we implemented Rubocop far beyond what's reasonable > point. > >> > > >> > It > >> > > >> > > > > > make sense for dangerous constructs, but not in this case > (and few > >> > > > > > others). > >> > > > > >> > > > +1, since rubocop leads to bike-shedding and annoyed devs from > both > >> > > >> > sides > >> > > >> > > > I > >> > > > don't think it's a good idea to introduce more cops. > >> > > > > >> > > > > I'd argue the opposite, in foreman_docker or foreman_ansible I > think > >> > > > > rubocop (with *all* cops enabled) helped maintain good coding > >> > > >> > standards > >> > > >> > > > > immensely and making the project *much easier* to read and get > used > >> > > >> > to. > >> > > >> > > > With all respect, these plugins do not have as many contributors > as > >> > > > Foreman > >> > > > and I'd like to hear from these contributors whether rubocop > helped or > >> > > > annoyed > >> > > > them. I don't think that all cops enabled == good coding > standards. > >> > > >> > And it > >> > > >> > > > does not imply good readability, that's on reviewer. TBH I think > it's > >> > > >> > also > >> > > >> > > > pretty subjective (remember explicit return cop discussion). > >> > > > > >> > > > -- > >> > > > Marek > >> > > > > >> > > > On Friday 19 of August 2016 11:54:23 Daniel Lobato Garcia wrote: > >> > > > > On 08/19, Lukas Zapletal wrote: > >> > > > > > > As discussed on IRC yesterday there should be consistency > and > >> > > >> > there > >> > > >> > > > is > >> > > > > >> > > > > > > an > >> > > > > > > option to autofix with rubocop if the style is changed to > change > >> > > > > > > existing > >> > > > > > > code with less effort. > >> > > > > > > >> > > > > > TL;DR - Let's keep Rubocop away from rockethash thing. > >> > > > > > > >> > > > > > What the consistency gives us? We all know there are two ways > and > >> > > >> > both > >> > > >> > > > > > will work. Let's avoid big bangs that will make cherry picking > >> > > >> > harder > >> > > >> > > > > > and just let's slowly improve as the time goes on. > >> > > > > > >> > > > > Not sure what cherry-picking becomes harder after this change? > It's > >> > > >> > just > >> > > >> > > > > that people might have to rebase their PRs? That's a small > price to > >> > > >> > pay > >> > > >> > > > > considering code is read 1000x more often than written. > >> > > > > > >> > > > > > I see no point in changing a single line of code from old to > new > >> > > > > > syntax > >> > > > > > just for that. We should only change it when changing logic. > >> > > > > > > >> > > > > > Even if Rubocop is able to check only for changed lines, I > won't > >> > > >> > like > >> > > >> > > > > > that at all. I do not want to switch my brain between Smart > Proxy > >> > > >> > and > >> > > >> > > > > > Foreman Core codebases. Both ways should work and be accepted. > >> > > >> > Let's > >> > > >> > > > > > only make the old syntax preferable when reviewing and that's > it. > >> > > > > > >> > > > > Precisely that's the painpoint, reviewers shouldn't have to pay > >> > > > > attention to that. One of the points for having style > guidelines is > >> > > >> > that > >> > > >> > > > > the code looks and reads as if it had been written by one > person. > >> > > > > > >> > > > > Think of it from the POV of the ocassional contributor who is > just > >> > > > > confused about which syntax to use because the code mixes them > both > >> > > >> > for > >> > > >> > > > > no reason. > >> > > > > > >> > > > > > I think we implemented Rubocop far beyond what's reasonable > point. > >> > > >> > It > >> > > >> > > > > > make sense for dangerous constructs, but not in this case > (and few > >> > > > > > others). > >> > > > > > >> > > > > I'd argue the opposite, in foreman_docker or foreman_ansible I > think > >> > > > > rubocop (with *all* cops enabled) helped maintain good coding > >> > > >> > standards > >> > > >> > > > > immensely and making the project *much easier* to read and get > used > >> > > >> > to. > >> > > >> > > > > Again I think we're really bikeshedding when the purpose of a > style > >> > > > > guide is to stop it and make code look more homogeneous > >> > > > > > >> > > > > > -- > >> > > > > > Later, > >> > > > > > > >> > > > > > Lukas #lzap Zapletal > >> > > > > > > >> > > > > > -- > >> > > > > > You received this message because you are subscribed to the > Google > >> > > > > >> > > > Groups > >> > > > > >> > > > > > "foreman-dev" group. To unsubscribe from this group and stop > >> > > >> > receiving > >> > > >> > > > > > emails from it, send an email to > >> > > > > > [email protected]. For more options, > visit > >> > > > > > https://groups.google.com/d/optout. > >> > > > > > >> > > > > -- > >> > > > > Daniel Lobato Garcia > >> > > > > > >> > > > > @dLobatog > >> > > > > blog.daniellobato.me > >> > > > > daniellobato.me > >> > > > > > >> > > > > GPG: http://keys.gnupg.net/pks/lookup?op=get&search= > >> > > >> > 0x7A92D6DD38D6DE30 > >> > > >> > > > > Keybase: https://keybase.io/elobato > >> > > > > >> > > > -- > >> > > > You received this message because you are subscribed to the Google > >> > > >> > Groups > >> > > >> > > > "foreman-dev" group. > >> > > > To unsubscribe from this group and stop receiving emails from it, > send > >> > > >> > an > >> > > >> > > > email to [email protected]. > >> > > > For more options, visit https://groups.google.com/d/optout. > >> > > >> > -- > >> > You received this message because you are subscribed to the Google > Groups > >> > "foreman-dev" group. > >> > To unsubscribe from this group and stop receiving emails from it, > send an > >> > email to [email protected]. > >> > For more options, visit https://groups.google.com/d/optout. > > > > -- > > You received this message because you are subscribed to the Google > Groups "foreman-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send > an email to [email protected]. > > For more options, visit https://groups.google.com/d/optout. > > -- > You received this message because you are subscribed to the Google Groups > "foreman-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "foreman-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
