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

Fixes Rails/IndexBy correction when .to_h is separated by a newline #239

Merged
merged 1 commit into from Apr 23, 2020
Merged

Fixes Rails/IndexBy correction when .to_h is separated by a newline #239

merged 1 commit into from Apr 23, 2020

Conversation

diogoosorio
Copy link

@diogoosorio diogoosorio commented Apr 22, 2020

I believe that the test cases included in this commit express well the
problem I'm trying to address, but running the auto-correction against
the following snippet of code:

x.map { |el| [el.to_sym, el] }.
  to_h

Would yield:

x.index_by { |el| el.to_sym }. # notice the ending "dot"

The output isn't parseable/valid Ruby. I belive the problem has to do
with the fact that correction code assumes that the .to_h statement is
always grouped together (which isn't true).

The proposed solution was to compute the trailing characters to be
stripped by subtracting the end position of the map block (the } character)
from end position of the whole expression.


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.

…newline

I believe that the test cases included in this commit express well the
problem I'm trying to address, but running the auto-correction against
the following snippet of code:

```rb
x.map { |el| [el.to_sym, el] }.
  to_h
```

Would yield:

```rb
x.index_by { |el| el.to_sym }. # notice the ending "dot"
```

The output isn't parseable/valid Ruby. I belive the problem has to do
with the fact that correction code assumes that the `.to_h` statement is
always grouped together (which might not be always true).

The proposed solution was to try to compute the number of trailing characters
to be stripped by subtracting the end position of the map block (the `}`
character) from end position of the whole expression.
@diogoosorio
Copy link
Author

diogoosorio commented Apr 22, 2020

Word of disclosure, this was pretty much the first time I've looked at this codebase (and attempted to understand the AST representation of the code) - it's likely that the approach is incorrect and in that case do "nudge" me in the right direction and I'll re-iterate on the patch (having a core contributor changing the PR directly is also fine by me 👍 ).

@koic
Copy link
Member

koic commented Apr 23, 2020

This approach seems to solve the issue nicely. Thank you for your patch!

@koic koic merged commit 7e05a1f into rubocop:master Apr 23, 2020
@koic
Copy link
Member

koic commented Apr 23, 2020

@diogoosorio RuboCop core's Style/HashTransformKeys and Style/HashTransformValues cops have the same problem. I opened rubocop/rubocop#7903 with you as a co-author. Thank you!

koic added a commit to koic/rubocop that referenced this pull request Apr 24, 2020
Follow up to rubocop/rubocop-rails#239.

This PR fixes an incorrect autocorrect for `Style/HashTransformKeys` and
`Style/HashTransformValues` cops when line break before `to_h` method.

In the future, duplicate code in RuboCop Rails may be removed,
making it dependent on RuboCop core code. It centralizes the resolution
of problems.

Co-authored-by: Diogo Osório <diogo.osorio@marleyspoon.com>
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.

None yet

2 participants