Turn off annoying cops

[fanaugen]

As discussed in a recent Backend chapter meeting, let’s collect here Rubocop rules that we don’t find helpful, i.e. rules that provide so little value that they’re not worth the effort to fix CodeClimate issues.

Then we can disable them for all Ruby projects.

Comment here with your favourite pointless cop that you’d love to retire! 🔥

[fanaugen]

@fnoeh commented:

“Align elsif with if ”, “Align else with if ”, etc.

Florian’s version:

def buyer_seller_relation
  @buyer_seller_relation ||= if same_country?
    :home
  elsif buyer_in_eu?
    :eu
  else
    :rest
  end
end

According to Rubocop, this is bad.

… they should be aligned with an if keyword

This is Rubocop’s preferred variant:

def buyer_seller_relation
  @buyer_seller_relation ||= if same_country?
                               :home
                             elsif buyer_in_eu?
                               :eu
                             else
                               :rest
                             end
end

Florian’s opinion: else , elsif etc. should not be aligned with the actual if keyword but have the same indentation level as the line with the if statement.

With ruby’s standard of 2 spaces per indentation level, in this example we are actually no longer using a multiple of 2 spaces because the consecutive lines are indented by 27 and 29 spaces in order to be aligned with if . That also means you cannot use Tab key to adjust indentation.

This is so awkward and I have never seen this used in any other programming language that programmer’s editors typically do not even support this kind of indentation.

[rubyconvict]

As I remember, you can please Rubocop and have Ruby (& editor) standard by doing this (also much cleaner in my opinion):

def buyer_seller_relation
  @buyer_seller_relation ||=
    if same_country?
      :home
    elsif buyer_in_eu?
      :eu
    else
      :rest
    end
end

Conclusion is that I would keep the cop.

[fanaugen]

Kill this rule
I think that forcing RSpec context descriptions to begin with "when" is a waste of time.

[rubyconvict]

This rule is crucial for understanding the output text (presentation of error/failure) when having nested contexts and for spotting the difference between closest context title and it block title. I'm not advocating nested contexts here, but I think we all use them sometimes, and that's when this rule shines.

[fnoeh]

But the rule prevents you from writing contexts like this:

context "when there is a shipping setting" do
  context "but it doesn’t have free_shipping_threshold set" do
    ...
  end 
end 

[kraflab]

Kill this. The example on that page is definitely bad, but this also prevents general organization of lets into positions that make the examples easier to read (i.e., putting the variables near the examples that need them).

[kraflab]

http://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MessageSpies

Code climate enforces have_received instead of receive for specs checking method calls. I have no idea why, and loveos-backend has 1155 instances of receive vs 55 instances of have_received, so I think there's a clear winner here.

[kraflab]

https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier

I'm all for doing a if b but the rule seems to ignore the size of the line right now. I'd rather have a block than a line that's 200 characters long.

[fanaugen]

A radical thought: how about we kill all cops that want to enforce the style of %w[array literals]? and leave it to the Ruby developers’ best judgment (famously this used to be Thoughtbot’s Policy Zero)?

I, for instance, am totally fine with using ['String', 'literals', 'in', 'brackets'] and %w(the percent notation) interchangeably. In my view, the former style is good for small lists, while the latter looks cleaner with large lists spanning several lines and likely to be updated frequently, but I wouldn’t want to bother spending time fixing a literal one way or another – except CodeClimate/Rubocop are forcing me to.

In the extreme case that anyone should desire to introduce %i;exotic things;, this would simply be flagged up in code review.

[shushugah]

Closing this per decision made at backend chapter to create individual issues per rubocop rule
https://www.notion.so/dawanda/2018-02-13-_day-1-day-9864d9c9da9045598649e5fc40c8e36f#f13d62e1970141bc871cc377bb5a694a

[fnoeh]

I would like to keep this open so that we have time to open new issues for the things mentioned here.
As you can see there are requests for changes here that show controversial opinions and others that don’t trigger any reaction at all. All of this in this single issue leads to nothing.

Let's try this process if you want a rule changed:

1. Create a new issue in this project where you
   a. Show the problem
   b. Give reasoning why it should be changed
   c. Optional: the necessary changes to .rubocop.yml
2. RFC in slack or chapter meeting so that others can give their opinion
3. A decision is made by a mediator (e. g. chapter leads, Dirk (worst case last resort), …) in the interest of all of us to come up with a sensible set of rules.

[juriglx]

[Turn Off] ExpectInHook

[fanaugen]

@fnoeh 👍 for reopening this.

Create a new issue

Why not a new PR?

A decision is made by a mediator (e. g. chapter leads, Dirk[…])

This sounds like a lot of operational overhead to me; how about – to keep this lightweight – we instead require one approval by a reviewer, just like in other repos, and if someone is opposed to a change, they can request changes in the PR. If no consensus is found, then we can escalate to an arbitrator…

[shushugah]

RIP 👮‍♂️