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.

Sign up for GitHub

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

Jump to bottom

[Fix #7291] Add better message for layout/HashAlignment #7547

Merged
merged 2 commits into from Dec 30, 2019
Merged

[Fix #7291] Add better message for layout/HashAlignment #7547

merged 2 commits into from Dec 30, 2019

Conversation

jsartin513
Copy link
Contributor

@jsartin513 jsartin513 commented Dec 3, 2019 •
edited

Fixes #7291

Before this change, the Layout/HashAlignment cop always gave the same error message: "Align the elements of a hash literal if they span more than one line."
This was confusing in situations where a hash appeared to be aligned, but was not according to the configured alignment settings.

This PR changes the error message to reflect which part(s) of a hash must be aligned to pass the cop.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

buehmann
buehmann reviewed Dec 3, 2019
View reviewed changes
lib/rubocop/cop/layout/hash_alignment.rb Outdated
@@ -248,10 +248,17 @@ def check_pairs(node)
add_offences
end

def specific_message(format)
messages = { KeyAlignment => 'Align the keys of a hash literal if they span more than one line.',
SeparatorAlignment => 'Align the separator of a hash literal if they span more than one line.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this read separators? (matching they)

lib/rubocop/cop/layout/hash_alignment.rb Outdated
TableAlignment => 'Align the keys and values of a hash literal if they span more than one line.'}
return messages[format]
end

def add_offences
_format, offences = offences_by.min_by { |_, v| v.length }
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that _format is no longer unused, better drop the underscore from its name.

@jsartin513
Copy link
Contributor Author

@buehmann I've addressed your feedback, I would appreciate if you could take another look!

I'm also getting an AppVeyor check failure, I think this is because of merge conflicts in the changelog. I think it makes sense to fix that when I'm otherwise ready to merge.

buehmann
buehmann suggested changes Dec 24, 2019
View reviewed changes
Copy link
Contributor

@buehmann buehmann left a comment

Choose a reason for hiding this comment

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

When you rebase, please pay attention to move your entry to the new master (unreleased) section. There has been a release 0.78.0 since you first created the PR.

lib/rubocop/cop/layout/hash_alignment.rb Outdated
@@ -248,10 +248,20 @@ def check_pairs(node)
add_offences
end

def specific_message(format)
Copy link
Contributor

Choose a reason for hiding this comment

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

The MSG constant near the top of the class can be removed then, can't it?

lib/rubocop/cop/layout/hash_alignment.rb Outdated
SeparatorAlignment => 'Align the separators of a hash '\
'literal if they span more than one line.',
TableAlignment => 'Align the keys and values of a hash '\
'literal if they span more than one line.' }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a constant. I think it would be better to treat it as such, that is, extract it as MESSAGES from the method.

And it would be great if you could add spaces in front of the backslashes and indent the second lines of the string literals. We do not have a cop for this yet, but see #6420. Something like this:

        MESSAGES = {
          KeyAlignment => 'Align the keys of a hash literal ' \
            'if they span more than one line.',

@jsartin513 jsartin513 force-pushed the feature/layout-alignhash-improvement branch 3 times, most recently from fdf5105 to 3bd6b2e Compare December 27, 2019 15:22
@jsartin513
Copy link
Contributor Author

Thanks @buehmann ! I've addressed your feedback, getting some test failures on CircleCI that seem unrelated (a local rake default looks fine, and these errors existed in some previous runs). Any suggestions on debugging that, or should I leave it alone for now?

@buehmann
Copy link
Contributor

@jsartin513 Thanks! I believe those test failures will be fixed by #7597

@jsartin513
Copy link
Contributor Author

Great, thank you @buehmann .

Is there anything else I should do to push this forwards, or do you take it from here?

@koic
Copy link
Member

koic commented Dec 27, 2019

Yeah, #7597 has been merged.

buehmann
buehmann approved these changes Dec 27, 2019
View reviewed changes
Copy link
Contributor

@buehmann buehmann left a comment

Choose a reason for hiding this comment

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

As a next step we need somebody from the core team to visit this PR, such as @bbatsov or @koic.

@koic
Copy link
Member

koic commented Dec 27, 2019

@jsartin513 Can you rebase with the latest master branch?

@jsartin513 jsartin513 force-pushed the feature/layout-alignhash-improvement branch from 3bd6b2e to 2986c54 Compare December 27, 2019 17:05
@jsartin513
Copy link
Contributor Author

@koic Done and all checks are passing now!

koic
koic reviewed Dec 28, 2019
View reviewed changes
CHANGELOG.md Outdated
@@ -11,6 +11,7 @@
* [#7193](https://github.com/rubocop-hq/rubocop/issues/7193): Prevent `Style/PercentLiteralDelimiters` from changing `%i` literals that contain escaped delimiters. ([@buehmann][])
* [#7590](https://github.com/rubocop-hq/rubocop/issues/7590): Fix an error for `Layout/SpaceBeforeBlockBraces` when using with `EnforcedStyle: line_count_based` of `Style/BlockDelimiters` cop. ([@koic][])
* [#7569](https://github.com/rubocop-hq/rubocop/issues/7569): Make `Style/YodaCondition` accept `__FILE__ == $0`. ([@koic][])
* [#7291](https://github.com/rubocop-hq/rubocop/issues/7291): Fix misleading error messages in `Layout/AlignHash` cop to specify how to align the hash. ([@jsartin513][])
Copy link
Member

Choose a reason for hiding this comment

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

I think changelog entry is unnecessary because this update does not change a behavior of the Layout/HashAlignment cop feature.

@bbatsov bbatsov merged commit 371da87 into rubocop:master Dec 30, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented Dec 30, 2019

Thanks for tackling this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koic koic koic left review comments

@buehmann buehmann buehmann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Layout/AlignHash pops up on aligned hash
4 participants
@jsartin513 @buehmann @koic @bbatsov