Skip to content

Conversation

@gfaza
Copy link

@gfaza gfaza commented Jan 16, 2025

adds rubocop layout configuration to enforce and allow for auto-correction of hash and array layouts.

for example, this diff demonstrates the auto-corrections and enforcement driven by these Cop settings when executing bundle exec rubocop -a --only Layout on web.

Copy link
Contributor

@ColinDKelley ColinDKelley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple questions.

Enabled: true
AllowForAlignment: true
AllowBeforeTrailingComments: true
ForceEqualSignAlignment: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to force equals alignment? I think we do that about 1/3 of the time, and 2/3 we don't? I'd most like to allow it, if that's a choice.

Copy link
Author

@gfaza gfaza Jan 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll take a look at what options are available on EqualSignAlignment.

full disclosure: i am biased toward always aligning consecutive assignments, feeling that it's more readable.

at some point though, I started favoring the forcing of alignment in order to force a standard, hoping to avoid any given chunk of code from churning back and forth between alignment and non-alignment, entirely depending on the last developer's IDE auto-format settings or personal preference.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to allow equal sign alignment only as optional, then auto-correction could look worse than without auto-correction. this is because optional alignment allows for "over-spaced" statements while desiring to reduce only "under-spaced" statements. e.g.,

experimentally the difference between the two settings of ForceEqualSignAlignment is as follows:

a setting of ForceEqualSignAlignment: false will remove extra spacing from "under-spaced" statements (which may be worse than leaving it alone) and do nothing about "over-spaced" statements
image

whereas a setting of ForceEqualSignAlignment: true will align both "under-spaced" and "over-spaced" statements
image

this example was actually inspired by a section of code that shows why I prefer forcing a standard over enforcing no standard, and how distracting it can be when reading and/or reviewing code.

https://github.com/Invoca/web/blob/748b6c1e2a9de442b9bd8f1d408fc25581a1d5ed/app/controllers/concerns/campaigns/affiliate_campaign_summary.rb#L81-L83

image

Enabled: true
EnforcedHashRocketStyle: table
EnforcedColonStyle: table
EnforcedLastArgumentHashStyle: always_inspect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables the alignment of hash key/value pairs for hashes that use either rocket or colon-style separators.

The last directive includes the application of this styling to hashes that appear as the last argument of a method call (whether or not the hash in that position explicitly is surrounded by braces or is implicitly omitting the surrounding braces).

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

Successfully merging this pull request may close these issues.

3 participants