standardrb / standard

Ruby's bikeshed-proof linter and formatter 🚲
Other
2.69k stars 209 forks source link

Case against AllowForAlignment: false #594

Closed schmijos closed 9 months ago

schmijos commented 9 months ago

In https://github.com/standardrb/standard/issues/127 you decided to rule alignment in our base configuration.

Layout/ExtraSpacing:
  AllowForAlignment: false

I didn't find an argument about why we should not use spaces for alignment. Can you point me somewhere?

While I never had the feeling that this rule limits me for assignments, I feel it's leaky in pattern matching cases where

case JSON.parse(message)
in ["EVENT", event]                   then handle_event(event)
in ["REQ", subscription_id, *filters] then handle_req(subscription_id, filters)
in ["CLOSE", subscription_id]         then handle_close(subscription_id)
else
  # Handle unknown message type
end

is auto fixed to

case JSON.parse(message)
in ["EVENT", event] then handle_event(event)
in ["REQ", subscription_id, *filters] then handle_req(subscription_id, filters)
in ["CLOSE", subscription_id] then handle_close(subscription_id)
else
  # Handle unknown message type
end

With such alignment I basically only care whether it's consistent per paragraph. I feel spacing actually transports meaning and I dislike the linting error "Unnecessary spacing detected."

searls commented 9 months ago

One principle of Standard's design is that it should enforce Ruby that can be written top-to-bottom in a code listing free of context of later lines, meaning that, wherever possible, the content of a later lines should never force a change in formatting of a prior line. Authors (whether human or the autofixer) should never have to backtrack and re-align code based on the content of code that comes later.

Formatting alignment of assignments and conditions in this way puts every line at the mercy of the length of the subsequent line, which—if not for the autofixer—would require someone to manually adjust the alignment of every prior line and, a secondary issue, result in version control churn on all lines when only one changed.

The simplest example of this principle is in Standard's banning of cute assignment alignment like this:

foo    = 1
barbaz = 2

And the same logic holds here So, in your example:

case JSON.parse(message)
in ["EVENT", event]                   then handle_event(event)
in ["REQ", subscription_id, *filters] then handle_req(subscription_id, filters)
in ["CLOSE", subscription_id]         then handle_close(subscription_id)
else
  # Handle unknown message type
end

If I were to add this pattern:

in ["WOAH_THIS_IS_A_REALLY_LONG_ONE_THERE_BOB", bob_id] then handle_bob(bob_id)

An alignment scheme like yours that had awareness of the surrounding lines in the expression would force the author (or the autofixer) to go back up and fix several lines that had already been written.

case JSON.parse(message)
in ["EVENT", event]                                     then handle_event(event)
in ["REQ", subscription_id, *filters]                   then handle_req(subscription_id, filters)
in ["CLOSE", subscription_id]                           then handle_close(subscription_id)
in ["WOAH_THIS_IS_A_REALLY_LONG_ONE_THERE_BOB", bob_id] then handle_bob(bob_id)
else
  # Handle unknown message type
end

Which represents a violation of this design principle

schmijos commented 9 months ago

Thank you for taking the time to explain. This makes a lot of sense in regards of version control and maintainability.

But one can take a contrary stance on that: especially with Ruby we value the here and now. We present code to the human reader as nicely as prose. In books we've got graphs, tables and images to convey information, but in code we don't. So I think your argument is the least applicable to Ruby. All other programming languages would benefit more.

I see and appreciate that you walk a risky ridge with standardrb and I'd like to know a bit more about this design principle. Was there an open discussion somewhere about the top-to-bottom design principle which I could read up? I only know how the Rails maintainers handle it (basically no enforced linters because of the point you made).

schmijos commented 7 months ago

I've got another argument to make PRO the design principle:

Tables are well suited to represent some information. But tables are not "responsive". There will always be a conflict between tables and lists. Code as-is cannot be aligned in tabular form (not even with tabs instead of spaces) while following the design principle.

So probably this is something the IDE should solve for you.

hopsoft commented 2 months ago

Not sure if anyone is following along, but I think you can disable this for certain areas with the following.

# rubocop:disable Layout/ExtraSpacing
# Aligned code here...
# rubocop:enable Layout/ExtraSpacing

It may make sense for some uses cases.