makandra / makandra-rubocop

makandra's default Rubocop configuration
MIT License
6 stars 1 forks source link

New cop Layout/MultilineArrayLineBreaks #10

Closed denzelem closed 4 years ago

denzelem commented 4 years ago

Rubocop added the cop Layout/MultilineArrayLineBreaks in version 0.67.

# bad
[
  a, b,
  c
]

# good
[
  a,
  b,
  c
]

We sometimes used things like this in the past:

attributes = %w[so many attribute here so we just wrap it to the new
line]

attributes = 
  %w[so many attribute here so we just wrap it to the new
         line]

I think the biggest disadvantage is adding or removing elements from something like this above (you might need to move other elements from one line to another), so enabling would make sense here. Note that the auto-correct fail for example like above, you need to manually adjust it.

I'm happy to hear your opinions.

dastra-mak commented 4 years ago

I understand why one might want that, as it's easier to see changes in the git history. On the other hand I don't care too much. Enabling or disabling are both fine for me.

foobear commented 4 years ago

I really don't like Arrays formatted like the "bad" example and I don't see any reason for that.

Keep the scope to understand your code low, don't force other developers to fully read your code to discover hidden secrets.

codener commented 4 years ago

This cop ensures that each item in a multi-line array starts on a separate line.


Con: Consider this array which holds parameters to be used in a Rails controller:

  NEWSLETTER_ATTRS = [
    :receives_newsletter, :newsletter_frequency, :preferred_language, :captcha_token,
    { topics: [] }
  ].freeze

I'd like to separate the scalar and the nested attributes by giving them their own line. Having all in the same line makes it easy to miss the topics, whereas moving each entry to its own line would unnecessarily push consecutive code down.

  PROFILE_ATTRS = [
    :salutation, :first_name, :last_name, :email,
    :current_password, :password, :password_confirmation,
    :job_title, :custom_job_title, :medium, :ressort, :country_id, :employment,
    :terms_of_service
  ].freeze

Here I have loosely grouped attributes by their semantics: "naming", "password", "job details". The array does not take up too much space while still being understandable. The same arguments as above apply.

foobear commented 4 years ago

@codener - Grouping is always subjective to some extent. If you group, I suggest helping out readers by clarifying what you are doing through a comment or by using variable names.

In your example it's hard to understand why country_id would not be grouped with other address fields like name.

It's not like you'd need to conserve line breaks (if you try to avoid your long class from becoming longer, maybe the problem is the class, not the list :wink:), so here is something that's more easily navigatable for a human:

PROFILE_ATTRS = [
  # Address
  :salutation,
  :first_name,
  :last_name,
  :email,

  # Password fields (may be left blank)
  :current_password,
  :password,
  :password_confirmation,

  # Employment
  :job_title,
  :custom_job_title,
  :medium,
  :ressort,
  :country_id,
  :employment,

  # TOS agreement is required to sign up
  :terms_of_service
].freeze

Also, I don't see a reason for grouping/separating scalar and nested attributes. Identifiers should be relevant, not their data types.

kratob commented 4 years ago

I don't mind @codener's examples. I think there are situations where you don't increase the readability of the class as a whole by making an array take up so much vertical space.