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

Use index_by and index_with wherever possible #38646

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

eugeneius
Copy link
Member

Using index_by or index_with is more concise than each_with_object and more performant than map { ... }.to_h or Hash[map { ... }].

I found these by writing a RuboCop rule, which I've submitted to the RuboCop Rails project in rubocop/rubocop-rails#208.

Using `index_by` or `index_with` is more concise than `each_with_object`
and more performant than `map { ... }.to_h` or `Hash[map { ... }]`.
@eugeneius eugeneius force-pushed the index_by_index_with branch from b69df9c to d3599d8 Compare March 5, 2020 01:24
@kaspth kaspth merged commit aeac2f5 into rails:master Mar 5, 2020
@kaspth
Copy link
Contributor

kaspth commented Mar 5, 2020

Dig this! They really are more concise. I’d be curious if Ruby might accept them in core 😄

@rafaelfranca
Copy link
Member

I had to wrap my head to understand what was going on with those method. Even the documentation of index_with is not clear enough.

@kaspth
Copy link
Contributor

kaspth commented Mar 5, 2020

True, they do take a little bit of learning. I find it quite understandable now though and the benefits far outweigh that initial cost. My mind sees it as index means hash from Enumerable, then by means derive the keys and with means derive the values. Haven’t looked at the index_with documentation since I wrote it, true it really only clicks once you get index_by, and there was contention around the naming of course. Again, I could live with that because it’s better than what was before.

@eugeneius
Copy link
Member Author

Upstreaming index_by was proposed in https://bugs.ruby-lang.org/issues/8490, but rejected; Matz didn't like that in this case "index" means "key" when elsewhere it refers to an array offset.

Enumerable#associate, which is effectively index_with, was proposed seven years ago in https://bugs.ruby-lang.org/issues/7341 but has never received a response from Matz.

Maybe a new proposal would be more successful, but it seems unlikely that the current names would be accepted. I'll think about some alternatives.


@rafaelfranca do you find the index_by changes less clear, or just the index_with ones? I remember that I found index_by intuitive the first time I saw it, but index_with took a while.

I've tried to clarify the docs for both methods in #38677.

@rafaelfranca
Copy link
Member

I find the concept of Array.index_* resulting in a Hash confusing. But the docs clarify it.

@koic
Copy link
Contributor

koic commented Mar 23, 2020

@eugeneius FYI, RuboCop Rails 2.5.0 including rubocop/rubocop-rails#208 has been released. Thank you for your great code!
https://github.com/rubocop-hq/rubocop-rails/releases/tag/v2.5.0

@eugeneius
Copy link
Member Author

Thanks for the heads up @koic 😊 I've opened #38992 to propose enabling the new cops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants