-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow empty when
clause for case
statements with an else
branch
#3696
Comments
@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. 😀 |
started working on this |
Awesome @hanumakanthvvn! Feel free to ping me if you have any questions. 😊 |
@Drenmi Thank you, so this check should be at node level right? or it should reflect to At present am changing the 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 |
@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:
WDYT? 😀 |
@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? |
@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 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 |
@Drenmi Yes you are right the example you given is not catchable by any other cop. I will look into. 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' |
… when body has comments
… when body has comments
I'm not 100% percent this is the case - probably make cops ignore comments completely. Frankly I'm not a big fan of implicit |
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. 😀 |
@hanumakanthvvn How far did you get with making comments count as contents? |
Indeed, it is possible to make this offense go away by replacing the empty body with By the way, given that the value of the |
I don't buy the explicit |
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! |
This issues been automatically closed due to lack of activity. Feel free to re-open it if you ever come back to it. |
Can we re-open this issue? It takes me a few seconds to process that an explicit |
I too would like this reopened. A comment explaining why the when-clause is empty is better than |
This looks like it makes sense. I've opened PR #7907. |
Fixes rubocop#3696 and rubocop#3754. This PR add `AllowComments` option to `Lint/EmptyWhen` cop. This option is enabled by default based on user feedback. It is also the same default as the option of `Lint/SuppressedException` set in rubocop#7805.
[Fix #3696] Add `AllowComments` option to `Lint/EmptyWhen` cop
Lint/EmptyWhen reports an offence for the following code, but the empty
when
branch is there to prevent going into theelse
branch. What would the "right" way be to express this?Expected behavior
Do not report an offence.
Actual behavior
Reported offence:
RuboCop version
0.45.0 (using Parser 2.3.1.4, running on ruby 2.3.1 x86_64-darwin14)
The text was updated successfully, but these errors were encountered: