Skip to content
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

AllowAlignment or ForceAlignment for single-line if/elsif/else style #10349

Open
nevans opened this issue Jan 5, 2022 · 1 comment
Open

AllowAlignment or ForceAlignment for single-line if/elsif/else style #10349

nevans opened this issue Jan 5, 2022 · 1 comment

Comments

@nevans
Copy link
Contributor

nevans commented Jan 5, 2022

Is your feature request related to a problem? Please describe.

I just upgraded a very large project from 1.13 to 1.24. Most of the offenses auto-corrected code that was written in 2013-2016 to use newly available features. There were only two rules I disagree with which had over 10 offenses. But they are both Lint rules and I hate to disable those too swiftly. I try to take lint rules very seriously before disabling them.

One of them is Lint/AmbiguousOperatorPrecedence and I agree with much of the feedback in #10080.

The other is Lint/ElseLayout. I see what the rule is getting at, but... the style I've been (manually) enforcing doesn't have that issue, IMO. I found #10147 and I agree with the feedback there and the proposed solution (allow single-line else when the if/elsif before it are single line) seems good to me, once it's working. But IMO, the rationale behind the original lint rule still applies to the examples given in that discussion.

To steal the simple example at the very top of #10147:

if number == 1 then puts "One"
elsif number == 2 then puts "Two"
else "Unknown"
end

IMO, this visual alignment of conditions and results is confusing when it comes to the else clause. And IMO this is just as true for the else clause in case/when/else statements.

Describe the solution you'd like

When I have a case statement or a chain of if/elsif/else statements which are all (or almost all) able to fit within 80 cols, I will usually convert it to the single-line style... but only if I can align all of the then clauses. e.g:

# this is how I would handle the example above
if    number == 1 then puts "One"
elsif number == 2 then puts "Two"
else                   "Unknown"
end
# and... aha!  After alignment, I think I found a "bug" in our toy code.

# what our else clause _probably_ meant to do was the following:
if    number == 1 then puts "One"
elsif number == 2 then puts "Two"
else                   puts "Unknown" # alignment highlights the missing puts
end

See how helpful alignment is for this specific use case? 🙂 Like I said above, I don't just allow alignment, I require it for all single-line else clauses. I also require it in case/else statements.

Here are a couple of other examples of what I'd consider "good" layout:

  def guess_imap_server
    if    gmail?;    "imap.gmail.com"
    elsif outlook?;  "imap-mail.outlook.com"
    elsif yahoo?;    "imap.mail.yahoo.com"
    elsif icloud?;   "imap.mail.me.com"
    elsif fastmail?; "imap.fastmail.com"
    elsif aol?;      "imap.aol.com"
    elsif in_db?;    db_lookup.imap_server
    elsif srv_dns?;  rfc6186_lookup.imaps
    else             "imap.#{domain}"
    end
  end

        token = lookahead                                                                                                                
        case token.symbol                                                                                                                
        when T_PLUS; continue_req                                                                                                          
        when T_STAR; response_untagged                                  
        else         response_tagged                                                                                                       
        end

In the if/elsif cases, not only do I require else clauses to be aligned, but also the original if condition.

Describe alternatives you've considered

Multiple times I've tried just never writing single-line if/elsif/else statements... I've tried not-aligning... and I can't do it! 😉 I crave the aligned tabular layout! It clarifies the code structure more than is possible with only indentation and keyword syntax highlighting. Consider:

  • cond foo
    • one
  • cond bar
    • two
  • cond baz
    • three

vs:

cond result
cond foo one
cond bar two
cond baz three

I'm sure this is more "style" than objective reality, and "de gustibus non est disputandum" ... but hopefully you can see how the tabular layout is a style some teams might want to enforce. I will gladly refactor code to name each conditional and each result with their own methods or procs, if it provides a clarifying tabular layout. But single-line conditional + result clauses without alignment... for me that's the worst of both worlds. 🙂

So, if I can fit everything and align it, I prefer single-line style. If I can't align (nearly) every row within my max columns, I usually convert all of them to "regular" style for both consistency and legibility. I do make a special exception for the else clause on its own line, because it's not unusual for the else clause to be an exception or otherwise special compared to the others.

Does that make sense?

@nevans
Copy link
Contributor Author

nevans commented Jan 5, 2022

FWIW: if someone gives me a few pointers to how to get started, I'd like to start learning how to make this sort of rule myself. Maybe I'll make a PR. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant