By the way, thanks to everyone that’s voted so far in the poll. If you haven’t voted yet, please do so:
http://goo.gl/forms/XQWiGlqrvof8yuWP2 I’ll leave it open until next week before I open a PR against Foreman. David On Wed, Jun 29, 2016 at 9:32 AM, Stephen Benjamin <[email protected]> wrote: > > > ----- Original Message ----- > > From: "Daniel Lobato Garcia" <[email protected]> > > To: [email protected] > > Cc: "David Davis" <[email protected]> > > Sent: Wednesday, June 29, 2016 6:17:45 AM > > Subject: Re: Re: [foreman-dev] Rubocop cops in Foreman > > > > I don't think the purpose of Rubocop or the style guide is really > > fulfilled if we start debating about most of them. > > > > 1. The purpose of the style guide is precisely that we don't argue > > anymore about this (bikeshedding, really) and we just stick to good > > practices (approved by a LOT of companies & individuals, including > > GitHub and Shopify which are 2 of the largest Rails projects out there) > > > > 2. 99% of cops are there for a good reason, including the method size, > > complexity ones, etc... if you feel strongly against any of these and > > you really have solid reasons, > > https://github.com/bbatsov/ruby-style-guide/issues > > is the place. > > > > Having or not having a style-guide is an important decision, > > debating every other rule is a triviality & detracts from its purpose. > > I think the effort to standardize on rubocop style across Katello/Foreman > is a good idea, and I think that's what David is trying to do here. Any > time > we get the two project orgs to adopt a common set of guidelines brings > us closer together and makes it a lot easier to work across multiple > projects. I > don't really think that's trivial. > > In fact, I'd like to see maybe a ruby skeleton repo that has things like > the rubocop config for our projects and the ability to sync them so > we can always be the same across the entire codebase. Having one style > would be great. > > There are cops that we HAVE to disable (or used to have to) like hash style > because of 1.8.7 compatibility. There are others whose thresholds are > too low for most of us (e.g. line length, method length/complexity, etc) > and > we seem to agree on those. There's plenty that I think are just annoying > and exist solely for the sake of existence because someone wanted to do > something cool with Rubocop and opened a PR. This isn't like PEP8 or other > coding styles where they seem pretty stable over time. > > Incidentally, Rails enables only a very small number of cops - and ALL the > rest > are off: > https://github.com/rails/rails/blob/master/.rubocop.yml > > > > > > On 06/28, Ivan Necas wrote: > > > Thanks David for the poll. It made me realize how terrible the code > > > would be if all the cops in rubocop were enabled :) > > > > > > -- Ivan > > > > > > On Thu, Jun 23, 2016 at 10:10 AM, Marek Hulán <[email protected]> > wrote: > > > > Thank you very much for this effort David. > > > > > > > > -- > > > > Marek > > > > > > > > On Wednesday 22 of June 2016 10:33:53 David Davis wrote: > > > >> Ok, created a poll for the cops that are disabled in Katello but are > > > >> outstanding in Foreman. I’ll create one PR for the ones we want to > > > >> disable > > > >> and then probably follow up with individual PRs for the ones we want > > > >> enabled. > > > >> > > > >> http://goo.gl/forms/XQWiGlqrvof8yuWP2 > > > >> > > > >> > > > >> David > > > >> > > > >> On Wed, Jun 22, 2016 at 8:24 AM, David Davis <[email protected] > > > > > >> wrote: > > > >> > I can do a poll. I actually thought about doing it before but > since I > > > >> > am > > > >> > not planning on actually enabling any of these cops the options > would > > > >> > be > > > >> > something like “disable” or “do nothing”. Would that make sense to > > > >> > people? > > > >> > > > > >> > > > > >> > David > > > >> > > > > >> > On Wed, Jun 22, 2016 at 4:50 AM, Ivan Necas <[email protected]> > wrote: > > > >> >> What about doing a poll for voting for the cops you're > suggesting to > > > >> >> disable? I'm afraid > > > >> >> the mail thread is not a suitable format for this kind of > questions. > > > >> >> > > > >> >> -- Ivan > > > >> >> > > > >> >> On Tue, Jun 21, 2016 at 6:03 PM, Stephen Benjamin > > > >> >> <[email protected]> > > > >> >> > > > >> >> wrote: > > > >> >>> ----- Original Message ----- > > > >> >>> > > > >> >>> > From: "David Davis" <[email protected]> > > > >> >>> > To: [email protected] > > > >> >>> > Sent: Tuesday, June 21, 2016 9:58:21 AM > > > >> >>> > Subject: Re: [foreman-dev] Rubocop cops in Foreman > > > >> >>> > > > > >> >>> > David > > > >> >>> > > > > >> >>> > On Tue, Jun 21, 2016 at 9:39 AM, Stephen Benjamin > > > >> >>> > <[email protected]> > > > >> >>> > > > > >> >>> > wrote: > > > >> >>> > > ----- Original Message ----- > > > >> >>> > > > > > >> >>> > > > From: "David Davis" <[email protected]> > > > >> >>> > > > To: [email protected] > > > >> >>> > > > Sent: Tuesday, June 21, 2016 8:59:54 AM > > > >> >>> > > > Subject: [foreman-dev] Rubocop cops in Foreman > > > >> >>> > > > > > > >> >>> > > > In order to have a more consistent rubocop configuration > > > >> >>> > > > across > > > >> >>> > > > >> >>> Foreman > > > >> >>> > > > >> >>> > > and > > > >> >>> > > > > > >> >>> > > > Katello, I’d like to bring some cops that Katello has > disabled > > > >> >>> > > > in > > > >> >>> > > > >> >>> its > > > >> >>> > > > >> >>> > > > rubocop configuration over to Foreman. These are cops that > > > >> >>> > > > we’ve > > > >> >>> > > > >> >>> decided > > > >> >>> > > > >> >>> > > > are a little bit too strict. > > > >> >>> > > > > > > >> >>> > > > Currently they are disabled in the rubocop todo file in > > > >> >>> > > > foreman > > > >> >>> > > > >> >>> meaning > > > >> >>> > > > >> >>> > > > they are *not* being enforced but they could potentially > be if > > > >> >>> > > > >> >>> someone > > > >> >>> > > > >> >>> > > > removes them from the todo file. > > > >> >>> > > > > > >> >>> > > It doesn't look like all of the ones below are disabled in > > > >> >>> > > foreman - > > > >> >>> > > > >> >>> e.g. > > > >> >>> > > > >> >>> > > redundant return, and lambda style. And quite a few of > them I > > > >> >>> > > like. > > > >> >>> > > > > >> >>> > Yea, I guess overlooked some cops. Let me compile a list with > the > > > >> >>> > ones > > > >> >>> > > > >> >>> that > > > >> >>> > > > >> >>> > are enabled in Foreman but disabled in Katello. Then we can > > > >> >>> > discuss if > > > >> >>> > > > >> >>> we > > > >> >>> > > > >> >>> > want to enable them in Katello or disable them in Foreman. > > > >> >>> > > > > >> >>> > > > I’m hoping to get some feedback as to which ones people > would > > > >> >>> > > > like > > > >> >>> > > > >> >>> *not* > > > >> >>> > > > >> >>> > > to > > > >> >>> > > > > > >> >>> > > > be disabled. I’ll collect the feedback and then open a PR > > > >> >>> > > > based on > > > >> >>> > > > >> >>> it. > > > >> >>> > > > >> >>> > > For > > > >> >>> > > > > > >> >>> > > > reference, here is our rubocop configuration in Katello: > > > >> >>> > > > > > > >> >>> > > > < > https://github.com/Katello/katello/blob/master/.rubocop.yml> > > > >> >>> > > > < > https://github.com/Katello/katello/blob/master/.rubocop.yml> > > > >> >>> > > > > https://github.com/Katello/katello/blob/master/.rubocop.yml > > > >> >>> > > > > > > >> >>> > > > And here are the cops I’d like to disable: > > > >> >>> > > > > > > >> >>> > > > Style/LeadingCommentSpace > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/Leading > > > >> >>> CommentSpace>>> > > > >> >>> > > > Style/IfUnlessModifier > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/IfUnles > > > >> >>> sModifier>>> > > > >> >>> > > > Style/RescueModifier > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/RescueM > > > >> >>> odifier>>> > > > >> >>> > > > Style/AssignmentInCondition > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Lint/Assignme > > > >> >>> ntInCondition>>> > > > >> >>> > > Why disable this? If you're doing assignment in a > conditional, > > > >> >>> > > it > > > >> >>> > > > >> >>> should > > > >> >>> > > > >> >>> > > be wrapped > > > >> >>> > > in parens to indicate your intention, e.g. if (foo = > 'bar'). If > > > >> >>> > > you > > > >> >>> > > > >> >>> don't > > > >> >>> > > > >> >>> > > do that, > > > >> >>> > > then I think rubocop should complain. This kind of bug > could go > > > >> >>> > > > >> >>> unnoticed > > > >> >>> > > > >> >>> > > if the normal > > > >> >>> > > case is for the `if` to evaluate as true. > > > >> >>> > > > > >> >>> > Originally this cop didn’t allow any assignments in conditions > > > >> >>> > > > >> >>> (regardless > > > >> >>> > > > >> >>> > of whether you use parentheses or not). However, it looks > like now > > > >> >>> > they > > > >> >>> > allow it so I’ll enable this cop in Katello and Foreman > (unless > > > >> >>> > there > > > >> >>> > > > >> >>> are > > > >> >>> > > > >> >>> > objections). > > > >> >>> > > > > >> >>> > > > Style/WhileUntilModifier > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/WhileUn > > > >> >>> tilModifier>>> > > > >> >>> > > > Style/AlignParameters > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/AlignPa > > > >> >>> rameters>>> > > > >> >>> > > > Style/ParenthesesAroundCondition > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/Parenth > > > >> >>> esesAroundCondition>>> > > > >> >>> > > > Style/DotPosition > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/DotPosi > > > >> >>> tion>>> > > > >> >>> > > > Style/Lambda > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/Lambda > > > >> >>> > > > >> >>> > > I thought we agreed to standardize on stabby lambda > everywhere > > > >> >>> > > > > >> >>> > I don’t remember seeing a discussion for this but I can leave > it > > > >> >>> > > > >> >>> enabled > > > >> >>> > > > >> >>> > unless anyone objects. > > > >> >>> > > > >> >>> I guess it wasn't really discussed on the list, the cop was just > > > >> >>> explicitly > > > >> >>> enabled and the one-liners all moved to the stabby type: > > > >> >>> https://github.com/theforeman/foreman/pull/2605 > > > >> >>> > > > >> >>> > > > Style/RedundantSelf > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/Redunda > > > >> >>> ntSelf>>> > > > >> >>> > > > Style/RedundantReturn > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/Redunda > > > >> >>> ntReturn>>> > > > >> >>> > > I like this cop > > > >> >>> > > > > >> >>> > Based on the outcome of this previous discussion ( > > > >> >>> > > https://groups.google.com/forum/#!topic/foreman-dev/77H7AN0wX4g) > > > >> >>> > I’ll > > > >> >>> > > > >> >>> leave > > > >> >>> > > > >> >>> > this as enabled and enable it in Katello. > > > >> >>> > > > > >> >>> > > > Style/SpaceInsideHashLiteralBraces > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/SpaceIn > > > >> >>> sideHashLiteralBraces>>> > > > >> >>> > > > Style/SingleLineBlockParams > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/SingleL > > > >> >>> ineBlockParams>>> > > > >> >>> > > > Style/Next < > > > >> >>> > > > >> >>> http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/Next> > > > >> >>> > > > >> >>> > > > Style/FormatString > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/FormatS > > > >> >>> tring>>> > > > >> >>> > > > Style/GuardClause > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/GuardCl > > > >> >>> ause>>> > > > >> >>> > > > Style/StringLiterals > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/StringL > > > >> >>> iterals>>> > > > >> >>> > > Ditto > > > >> >>> > > > > >> >>> > I don’t have a huge preference on this but I will say that it > is > > > >> >>> > going > > > >> >>> > > > >> >>> to > > > >> >>> > > > >> >>> > be a HUGE pain to fix all the places that mix single and > double > > > >> >>> > quotes. > > > >> >>> > Also, I find it kind of a pain to turn string literals into > > > >> >>> > strings > > > >> >>> > > > >> >>> that > > > >> >>> > > > >> >>> > can be interpolated. > > > >> >>> > > > >> >>> Ok, fair enough, I do agree it's a pain. For some reason I > thought > > > >> >>> using single > > > >> >>> quotes was faster since it didn't have to look for > interpolation but > > > >> >>> the > > > >> >>> internets > > > >> >>> seem to say there's no significant difference. > > > >> >>> > > > >> >>> > > > Style/WordArray > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Style/WordArr > > > >> >>> ay > > > >> >>> > > > >> >>> > > > Rails/ScopeArgs > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/github/bbatsov/rubocop/Rubocop/Cop/Rails/ScopeAr > > > >> >>> gs > > > >> >>> > > > >> >>> > > > Style/EachWithObject > > > >> >>> > > > < > > > >> >>> > > > >> >>> > http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/EachWithObject> > > > >> >>> > > > >> >>> > > > Style/SymbolProc > > > >> >>> > > > < > http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/SymbolProc > > > >> >>> > > > > > > >> >>> > > > > > > >> >>> > > > Let me know if there are any questions. Thanks. > > > >> >>> > > > > > > >> >>> > > > David > > > >> >>> > > > > > > >> >>> > > > -- > > > >> >>> > > > 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. > > > > > > > > -- > > > > 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. > > > > -- > > 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.
