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

[Fix #6936] Fix MultilineMethodArgumentLineBreaks when bracket hash assignment on multiple lines #7072

Merged
merged 1 commit into from May 27, 2019

Conversation

maxh
Copy link
Contributor

@maxh maxh commented May 22, 2019

Fixes #6936 by handling this case:

a['b'] = c(
  1,
)

The [] assignment is represented in the AST by a send node with b as an argument and the expression on the right as an argument. It's ok that these are on the same line.


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.

@mikegee
Copy link
Contributor

mikegee commented May 22, 2019

I support merging this as is, since it definitely fixes a bug. 👍

I'm also concerned this PR allows for similar bug. Is the following code allowed incorrectly?

a['b',
  'c', 'd'] = e

@maxh
Copy link
Contributor Author

maxh commented May 23, 2019

I'm also concerned this PR allows for similar bug. Is the following code allowed incorrectly?

This PR fixes that bug as well. Added a spec to illustrate/cover that case.

@bbatsov bbatsov merged commit 958cf68 into rubocop:master May 27, 2019
@bbatsov
Copy link
Collaborator

bbatsov commented May 27, 2019

Thanks!

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.

Layout/MultilineMethodArgumentLineBreaks False Positive On Multiline Assignment
3 participants