Allow empty when clause for case statements with an else branch

[donv]

Lint/EmptyWhen reports an offence for the following code, but the empty when branch is there to prevent going into the else branch. What would the "right" way be to express this?

case status
when STATUS_RUNNING
  puts 'Process already running.  Stopping.'
  exit 1
when STATUS_STALE
  puts 'Deleting stale PID file.'
  File.delete(PID_FILE)
when STATUS_STOPPED
  # This is fine.  Do nothing.
when STATUS_NOT_ROOT
  puts 'Permission denied.  Please run as root/administrator.'
  exit 6
else
  puts "Unknown status: #{status}.  Stopping"
  exit 2
end

Expected behavior

Do not report an offence.

Actual behavior

Reported offence:

W: Lint/EmptyWhen: Avoid when branches without a body.

RuboCop version

0.45.0 (using Parser 2.3.1.4, running on ruby 2.3.1 x86_64-darwin14)

[Drenmi]

@donv: If you ask me, a node with a comment in it shouldn't be considered empty. (I think this is the convention we go with for other cops.) I'll see if I have time to amend this during this weekend. If someone else wants to pick it up, feel free to do so. 😀

[hanumakanthvvn]

started working on this

[Drenmi]

Awesome @hanumakanthvvn! Feel free to ping me if you have any questions. 😊

[hanumakanthvvn]

@Drenmi Thank you, so this check should be at node level right? or it should reflect to when clause at present?

At present am changing the empty_when_body?(when_node) in for /lib/rubocop/cop/lint/empty_when.rb as to avoid the when branches without the body

   def empty_when_body?(when_node)
     node_comment = @processed_source[when_node.loc.first_line]
     !(when_node.to_a.last || comment_line?(node_comment))
   end

[Drenmi]

@hanumakanthvvn: This works, but only if the comment is on the first line of the body. I think we should make it more robust than that, to cover the case where there's empty newlines:

when :bar

  # no-op
end

WDYT? 😀

[hanumakanthvvn]

@Drenmi Yes, but i thought of it violates the other cop rule of 'Extra blank line detected'. Do you want to skip the empty lines in this case?

[Drenmi]

@hanumakanthvvn: This is normally a judgement call, asking whether it ventures into the area of responsibility of the other cop. My example won't be caught by EmptyLines, because it checks for "two or more consecutive spaces."

In this case, I think we should check the entire body of the when node, since we otherwise produce a false positive if there's a single empty line, or if EmptyLines is not enabled.

[hanumakanthvvn]

@Drenmi Yes you are right the example you given is not catchable by any other cop. I will look into.
So here i need to handle if it is like

when :bar

  # no-op
end

but if it is like

when :bar
  # no-op

end

then it should be default warn like 'Extra blank line detected'
Am i on the right path?

[bbatsov]

@donv: If you ask me, a node with a comment in it shouldn't be considered empty. (I think this is the convention we go with for other cops.) I'll see if I have time to amend this during this weekend. If someone else wants to pick it up, feel free to do so. 😀

I'm not 100% percent this is the case - probably make cops ignore comments completely. Frankly I'm not a big fan of implicit nil returns, so this is not something I've considered to be an issue myself, but I understand your point of view. I just want people to always keep in mind there's not such thing as "do nothing" - it just a nil return.

[Drenmi]

I'm not 100% percent this is the case - probably make cops ignore comments completely. Frankly I'm not a big fan of implicit nil returns, so this is not something I've considered to be an issue myself, but I understand your point of view. I just want people to always keep in mind there's not such thing as "do nothing" - it just a nil return.

I actually personally agree with you, and I don't abide the use of unstructured comments. I remember working on some other cop, though, and there were complaints about this. I think (might be wrong on this) other cops consider nodes with comments "non-empty." If we want to change this and have "don't count comments" as a consistent rule, I'm not against it. 😀

[walles]

@hanumakanthvvn How far did you get with making comments count as contents?

[mvz]

I just want people to always keep in mind there's not such thing as "do nothing" - it just a nil return.

Indeed, it is possible to make this offense go away by replacing the empty body with nil, and indeed RuboCop reports no offense at all in that case.

By the way, given that the value of the case statement is not used, you can also put 42, again without any offense reported. This is a little surprising (although it may be hard to detect unused case statement result values).

[chewi]

I don't buy the explicit nil argument as you may not care about the return value in any of the cases.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

[stale]

This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it.

[eyqs]

Can we re-open this issue? It takes me a few seconds to process that an explicit nil is just a placeholder for a no-op block, as opposed to anything semantically meaningful. I'd be happy to work on this if others agree that having an empty block with a comment is OK.

[donv]

I too would like this reopened. A comment explaining why the when-clause is empty is better than nil which does not explain anything.

[koic]

This looks like it makes sense. I've opened PR #7907.