infinum / rubocop-infinum

Infinum's Ruby Style Guide
0 stars 1 forks source link

Hash alignment #5

Closed d4be4st closed 3 years ago

d4be4st commented 4 years ago

Proposal:

Layout/HashAlignment:
  EnforcedHashRocketStyle: table
  EnforcedColonStyle: table
# Default 
has_many :iqos_geolocations,
         class_name: 'Iqos::Geolocation',
         dependent: :destroy,
         foreign_key: :iqos_expert_id,
         inverse_of: :iqos_expert,
         autosave: true

# Table
has_many :iqos_geolocations,
         class_name:  'Iqos::Geolocation',
         dependent:   :destroy,
         foreign_key: :iqos_expert_id,
         inverse_of:  :iqos_expert,
         autosave:    true

Or

# Default 
value :expire,
      from: ::Iqos::Leads::Status.where(expireable: true).map(&:to_sym),
      to: :expired,
      automated: true,
      organizer: ::Iqos::Leads::Organizers::Events::ExpireOrganizer

# Table
value :expire,
      from:      ::Iqos::Leads::Status.where(expireable: true).map(&:to_sym),
      to:        :expired,
      automated: true,
      organizer: ::Iqos::Leads::Organizers::Events::ExpireOrganizer

Or

# Default
message(
  from_phone: Iqos::Leads::InfobipEndpointNumber.mgm2.number,
  to_phone: Settings.iqos.default_sms_recipient || params[:godfather].phone,
  type: Core::Messages::Type.iqos_godfather_blacklisted,
  body: I18n.t('messages.iqos.godfathers.blacklisted')
)

# Table
message(
  from_phone: Iqos::Leads::InfobipEndpointNumber.mgm2.number,
  to_phone:   Settings.iqos.default_sms_recipient || params[:godfather].phone,
  type:       Core::Messages::Type.iqos_godfather_blacklisted,
  body:       I18n.t('messages.iqos.godfathers.blacklisted')
)

table seems cleaner

d4be4st commented 4 years ago

cc @infinum/backend-team

cilim commented 4 years ago

Isn't something like this already supported by rubocop? What's the difference? (I personally don't use the table style)

d4be4st commented 4 years ago

Default is "#bad" examples

d4be4st commented 4 years ago

Updated the OP, to use # Default and # Table

JureCindro commented 4 years ago

I don't think the "cleanliness" is a good enough reason because:

  1. there will be more of line too long errors in cases where 1 of the keys is long
  2. even if IDE/editor is smart enough to know how much spaces to add after a key, adding a long key at the end of a hash will probably require you to reformat code or to run linters with autocorrect.
  3. changing the longest key will generate a lot of diff, it is whitespace diff which can be ignored with certain flags, but I do not know them from the top of my head, there is an option on github to show/hide whitespace diff, again it is a bandaid

Also about the cleanliness argument, the table view has its benefits which is that if you look at the whole hash, you can easily see all of the values or all of the keys. But since most of the work with hashes is looking at the value of a specific key, the more common way of viewing is key-value, column by column (as the intended store). I think it is easier to see the value of a key if they have just a single space in between them. Having a longer key would require you to use some kind of visual guidelines to easily look at the value of shorter keys, which is more taxing.