Lint/DuplicateBranch: Duplicate branch body detected -- for code it previously was happy with

[drnic]

I upgraded to standard 0.10/rails 6.1.0.rc2, running ruby 2.7.1:

-    standard (0.9.0)
-      rubocop (= 1.2.0)
-      rubocop-performance (= 1.8.1)
+    standard (0.10.1)
+      rubocop (= 1.4.2)
+      rubocop-performance (= 1.9.1)

And now the following method is receiving a warning that doesn't seem correct:

module AnnouncementsHelper
  # Use the explicit class names so purgecss can find them
  def announcement_color(announcement)
    case announcement.kind
    when "new"
      "announcement-new"
    when "update"
      "announcement-update"
    when "improvement"
      "announcement-improvement"
    when "fix"
      "announcement-fix"
    else
      "announcement-update"
    end
  end

The warning points to the else in the case statement above:

standard: Use Ruby Standard Style (https://github.com/testdouble/standard)
  app/helpers/announcements_helper.rb:13:5: Lint/DuplicateBranch: Duplicate branch body detected.

This new warning is in 0.10.0 and 0.10.1, but not 0.9.0

[drnic]

Discussion on rubocop's new Lint/DuplicateBranch quirks is at rubocop/rubocop#8404

[jmkoni]

I think the idea behind this is that when "update" is equivalent to the else, so it doesn't need to exist as a branch.

[jmkoni]

So it's expecting this:

def announcement_color(announcement)
    case announcement.kind
    when "new"
      "announcement-new"
    when "improvement"
      "announcement-improvement"
    when "fix"
      "announcement-fix"
    else
      "announcement-update"
    end
  end

[drnic]

@jmkoni ahh thanks a lot for spotting that. Obviously I stared too much at the else section and didn't go looking further. Sorry; thanks again.

[jmkoni]

@drnic no worries :) happy to help!

[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?

[rsanheim]

Proposal: PR to disable this rule: #228

[searls]

Hey @drnic, hey @rsanheim! Thanks for raising these issues. When @jmkoni & I were discussing this rule, this kind of risk of limiting the expressiveness of code (if two semantically unrelated branches happen to resolve to the same value, it's not always ideal to smoosh them together), but turned the rule on anyway. We turned it on as a bit of a trial balloon and you have successfully popped it. Thank you!

[klueless-io]

I'm getting this warning on the following code and the logic is quite correct in implementation and does not lend itself to be refactored based on your rule

W: Lint/DuplicateBranch: Duplicate branch body detected.
else
^^^^

@implementation_type = if @target_instance.nil?
                               :method
                             elsif match(Peeky::Predicates::AttrReaderPredicate)
                               :attr_reader
                             elsif match(Peeky::Predicates::AttrWriterPredicate)
                               :attr_writer
                             else # <<-- here
                               :method
                             end

[searls]

@klueless-io we've disabled this rule from Standard. You might want to make sure you've upgraded to latest and try again. Separately, if you can reproduce what you're seeing with a default rubocop config, I'm sure they'd appreciate the bug report