Proposal: disable Lint/DuplicateBranch

[rsanheim]

This cop just started hollering for us. I think this is a step back for a pretty common usage of case statements w/ enums. Here is an example from a code base I work in everyday:

  def opd_load_for_facility_size
    case facility_size
    when "community" then 450
    when "small" then 1800
    when "medium" then 3000
    when "large" then 7500
    else 450
    end
  end

In our domain the four types of facility sizes are a well know attribute on our Facility model. So when getting something like this opd_load attribute for a Facility, having the value explicitly returned for each size makes sense, even tho community is duplicated with the else clause. The else clause is handling some legacy cases for facility size (I believe, it predates me joining Simple.org) where we could have a nil size or perhaps just old, bad values.

So while the refactoring Rubocop is proposing is more DRY in the technical sense, the logic here is not duplicated. Refactoring to handle the 450 case entirely in the else clause is a step backwards in terms of our readability in our codebase. To take a cue from Sandi Metz, this rule pushes us towards the wrong abstract - Facilities with size community are not the same thing as the legacy facilities that are captured by the else clause here.

While I can see cases where you may want this cop enabled, I think this an overly strict definition of correctness that isn't in the spirit of standardrb. This is also way more than I've written about a single lint rule, so maybe I should just figure out how to turn off the rule for our project and move on 😏 .

But I think this should be off for the default looser world of standardrb - what say y'all?

[searls]

Hey Rob! It's great to hear from you and I hope you're doing well.

Yeah, this is a lights-out great reason to be against this rule, and I was already on the fence.

[searls]

For the record we just delete things from the YAML file when we remove this (I'll fix this in master). Reason being that there are so many rules out there and having to visually parse enabled/disabled stuff or commented rules is just too much work. If someone wants to know the sordid history of a particular cop they can just git log -S DuplicateBranch and find this commit!

[searls]

Landed in 0.10.2

[rsanheim]

Landed in 0.10.2

Thanks Justin!

Nice to hear from you too. Cheers, and thanks for the work on the great gem.

[bbatsov]

FYI - you're welcome to raise such concerns upstream as well. We welcome all feedback and we realize that often the first versions of some new cops are flawed in one way or another. We'll address that particular concern that was raised here soon.