[Fix #7705] Fix problem of Style/OneLineConditional cop with elsif.
#7710
Conversation
One line conditionals with elsif branches are now transformed into multiline versions. Before this change, they were broken and autocorrect produced code that resulted in a syntax error. See Issue rubocop#7705 for more details.
Lykos
changed the title
Style/OneLineConditional cop with elsif.
|
Friendly ping? |
| @@ -350,7 +350,7 @@ This cop can be configured with the EnforcedStyleForLeadingUnderscores | |||
| directive. It can be configured to allow for memoized instance variables | |||
| prefixed with an underscore. Prefixing ivars with an underscore is a | |||
| convention that is used to implicitly indicate that an ivar should not | |||
| be set or referenced outside of the memoization method. | |||
| be set or referencd outside of the memoization method. | |||
There was a problem hiding this comment.
Seems you unfixed a typo. :-)
| 'over `%<keyword>s/then/else/end` constructs.' | ||
| MSG_USE_TERNARY = 'Favor the ternary operator (`?:`) ' \ | ||
| 'over `%<keyword>s/then/else/end` constructs.' | ||
| MSG_USE_MULTILINE = 'Use multiple lines for ' \ |
There was a problem hiding this comment.
Seems to me that this change is more than a bugfix - it actually changes the behaviour of the cop. Why would we want to treat this case differently?
|
Sorry for the slow response. I've added one question, as I'm not sure that this is exactly a bugfix. Also - when doing functional changes you always have to update the cop documentation as well, but let's first agree on what exactly do we want to do. |
|
I agree about having to update the description. But note that this is really a bugfix even though it introduces new behavior. But it introduces new behavior only in the case that was previously broken. One line conditionals with an See the discussion on #7705. But since nobody reacted, I decided to just go ahead with the version that I considered most reasonable. |
Fair enough.
I think the fundamental problem was that we tried to make those ternary conditions instead of just make them multi-line, which is obviously a more generic solution to the problem. I'm wondering if by default we shouldn't just make everything multiline and have an additional flag to convert one line conditions to ternary ops only when feasible. I think that's going to make the cop way more consistent.
Sorry about this. Unfortunately there are many open tickets and we don't have the time to respond to all of them in a timely fashion. Overall I think your direction makes sense, but we need to make the cop's behaviour more uniform as stated above. |
|
@Lykos ping :-) |
|
I think I won't have time to do this any more, so consider this stale. |
One line conditionals with elsif branches are now transformed into multiline versions. Before this change, they were broken and autocorrect produced code that resulted in a syntax error. See Issue #7705 for more details.