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

Feature request: Layout/AlignHash option to allow for key and table styles to both be permissible #6410

Closed
mattbrictson opened this issue Oct 27, 2018 · 8 comments

Comments

@mattbrictson
Copy link

mattbrictson commented Oct 27, 2018

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

The changes to Layout/AlignHash in 0.60 mean that you can no longer mix the key and table style without one of them being reported as a violation. For example:

key_style = {
  key: :value,
  another: :value,
  yet_another: :value
}
table_style = {
  key:         :value,
  another:     :value
  yet_another: :value
}

Before 0.60, if you chose the key style, both hashes would be allowed, because in both cases the keys are left-aligned.

However starting with 0.60, RuboCop strictly enforces the whitespace to the right of the keys. If you set the enforced style to key, the second hash will get marked as a violation. If you choose table, then the first will be a violation.

I believe that there is room for both styles in a single codebase. For small hashes, key is appropriate. But for large hashes the table style is much easier to read. Take a look at the changes introduced by 0.60 to RuboCop's own codebase. I think many people would agree that readability has been decreased.

Describe the solution you'd like

I am open to ideas. A naive solution would be a key_or_table option that allows for both styles of whitespace to be accepted. I.e.

Layout/AlignHash:
  EnforcedColonStyle: key_or_table
  EnforcedHashRocketStyle: key_or_table

Describe alternatives you've considered

Currently the only way to mix key and table styles in the same codebase is to turn off AlignHash altogether. This is a big step backwards, though, because now even blatantly bad alignments will be allowed without any violations, like this:

{
  key: :value,
    another:   :value
}

Or I could set the enforced style to key, and then use a # rubocop:disable comment around every hash where I want to use table syntax. That is a bit tedious, and again, it disables alignment checking completely. To my knowledge there is not a way to change the enforced style with a # rubocop comment; you can only disable the cop.

@mattbrictson mattbrictson changed the title Feature request: Layout/Align option to allow for key and table styles to both be permissible Feature request: Layout/AlignHash option to allow for key and table styles to both be permissible Oct 27, 2018
@bquorning
Copy link
Contributor

I believe that there is room for both styles in a single codebase. […] Take a look at the changes introduced by 0.60 to RuboCop's own codebase. I think many people would agree that readability has been decreased.

I agree 100%, and was about to open an issue myself.

What I noticed after seeing the changes to this repository, and two others where I tried upgrading to rubocop v0.60.0 was: it seems that the table style is often used for “literal” hashes, while the key style is used when a hash makes part of method arguments. If I have time, I will try modifying AlignHash to enforce this style, so I can count the number of offenses in these three repositories I checked.

bquorning added a commit to rubocop/rubocop-rspec that referenced this issue Oct 27, 2018
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well desribed in
rubocop/rubocop#6410 .
bquorning added a commit to rubocop/rubocop-rspec that referenced this issue Oct 27, 2018
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
@bbatsov
Copy link
Collaborator

bbatsov commented Oct 28, 2018

I expected a ticket like this, as the change was somewhat controversial. :) Maybe we can add something which behaved like the old style, but I'm not sure about the naming there (or the exact behaviour it should have).

bquorning added a commit to rubocop/rubocop-rspec that referenced this issue Nov 1, 2018
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
@thomthom
Copy link
Contributor

thomthom commented Dec 6, 2018

Fully agree with the original post. I had to disable AlignHash on my own projects because readability of the hashes got reduced in many cases by forcing one or the other option.

What I found useful of this cop was that the key alignment was consistent.
Having value alignedment check would be nice if it is able to accept that values are consistently aligned either by 1. one space after key or left aligned with the rest of the values.

Via the AST that should be possible, right? After having collected the Hash literal, one could query the source ranges for key and value and match them against these criterias?

Not sure what the naming should be. key_or_tablesounds very accurate. While flexibleis much more ambiguous (would require looking up the docs to recall its meaning.)

@stale
Copy link

stale bot commented May 8, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution and understanding!

@stale stale bot added the stale Issues that haven't been active in a while label May 8, 2019
@mvz
Copy link
Contributor

mvz commented May 9, 2019

@bquorning have you been able to find the time to do your experiment?

@stale stale bot removed the stale Issues that haven't been active in a while label May 9, 2019
@bquorning
Copy link
Contributor

I did a small experiment, but as I recall, the results were inconclusive.

@stoivo
Copy link
Contributor

stoivo commented May 13, 2019

I think I have created a PR where you can allow both key and table at the same time. Take a look at #6649.

The rubocop yaml will look like:
https://github.com/rubocop-hq/rubocop/pull/6649/files#diff-11a0d7906801d9dea0eccb85667ad811

@mattbrictson
Copy link
Author

Closing as fixed by #6649

kellysutton pushed a commit to kellysutton/rubocop-rspec that referenced this issue Oct 28, 2019
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
pirj pushed a commit to rubocop/rubocop-capybara that referenced this issue Dec 29, 2022
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
ydah pushed a commit to rubocop/rubocop-factory_bot that referenced this issue Apr 13, 2023
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this issue Mar 27, 2024
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
ydah pushed a commit to rubocop/rubocop-rspec_rails that referenced this issue Mar 27, 2024
I disagree with the recent changes in AlignHash, so a combination of `key` and
`table` styles is no longer allowed. The issue is very well described in
rubocop/rubocop#6410 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants