On Wed, Aug 24, 2016 at 4:49 AM, Marek Hulán <mhu...@redhat.com> wrote:
> Few more comments below > > > What do you mean by "Foreman dev"? > > You, me, everyone who contributed to Foreman. > So then your statement about not one foreman dev being a rubocop contributor is incorrect, right? > > > 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. > > 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]. > > You’re grepping for lambda and only checking for a handful of lambda cops. What about cops like MethodCallParentheses, MultilineIfThen, etc? Even some of the cops like SpaceAroundOperators would have prevented stuff like “=->”. I would hope that all foreman and foreman-related projects would have some level of rubocop checking even if it’s just basic stuff like whitespace (I tried to do this for dynflow but closed my PR out after it had conflicts and no activity). > 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 That’s not true. The HashSyntax cop can be used to check for hash rockets: https://git.io/v67yi My vote is not to enforce hash syntax—let developers choose whether to use 1.9 vs hash rockets. Also, I am not a huge fan of Github’s style guide since it has some weird things like using TomDoc: https://github.com/styleguide/ruby/documentation > > > [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 <mhu...@redhat.com> 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 <mhu...@redhat.com> > 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 > > > > > > > foreman-dev+unsubscr...@googlegroups.com. 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 foreman-dev+unsubscr...@googlegroups.com. > > > > > 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 foreman-dev+unsubscr...@googlegroups.com. > > > 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 foreman-dev+unsubscr...@googlegroups.com. > 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 foreman-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
