Prefer each.with_index and each.with_object

[danielmorrison]

I have long held belief that we should encourage each.with_index over each_with_index. It comes from years of teaching Ruby. Someone would inevitably ask if there was a map_with_index (there isn't). I realized it was better to introduce each.with_index instead, so that they naturally would think to try map.with_index and find out that it Just Works™.

Rubocop nicely converts inject to each_with_object where it can, and this goes a step farther by converting it to each.with_object.

Before:

list.each_with_index do |item, index|
  puts item[index]
end

list.each_with_object do |item, result|
  result[item] = "something"
end

list.inject({}) do |result, item|
  result[item] = "something"
  result
end

After this patch, running --fix produces:

list.each.with_index do |item, index|
  puts item[index]
end

list.each.with_object do |item, result|
  result[item] = "something"
end

list.each.with_object({}) do |item, result|
  result[item] = "something"
end

This is a possibly unpopular hill that I'm willing to shed blood defending.

[danielmorrison]

My first version had a side effect of enabling preferences for other collection methods (collect, inject, etc.) Updated to keep those disabled.

[searls]

Why did you make these changes in ruby-2.2.yml instead of base.yml?

[danielmorrison]

Those didn’t come until 1.9 (I think). I’ll verify later if you haven’t by then. ;)

…

On Dec 8, 2018, at 9:56 AM, Justin Searls ***@***.***> wrote: Why did you make these changes in ruby-2.2.yml instead of base.yml? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[searls]

As to the substance of this issue (and the closely related but apparently-opposite-in-recommendation #41) I am a bit torn. I love me some consistency, but telling people which methods to call seems like it crosses a line.

To be honest I think it's a little bizarre for a linter to push people to invoke different methods or avoid an alias. The reason behind this feeling, I think, is because it seems pretty presumptive to say "no, user, you may have meant to say inject but I we are all using reduce now", especially because Standard doesn't give them a way to opt out.

Additionally (IIRC), RuboCop does not always perfectly make these corrections, so (using the same example) if inject happens to be a method invoked on a custom PORO, RuboCop might mistakenly change it to reduce and that'd be pretty super bad.

Ruby and (especially) Rails has an utterly massive surface area of methods and aliases to those methods and whether that's a strength or weakness depends largely on one's context. (Been a while since I linked to this, heh.) But regardless of the merits, Rubyland is littered with humane interfaces and it feels like it would be antithetical to what Ruby is to try to consistent-ify that by effectively banning alias use. Even if we agreed that it was a good idea, we'd never find them all.

Going to close this issue, but please feel welcome to change-my-mind.gif

[searls]

Those didn’t come until 1.9 (I think).

I’ll verify later if you haven’t by then. ;)

Ah, so the way the config is loaded is that first base is loaded, then the earlier targets are applied only if they're running on an older Ruby. So if this rule change is applied to ruby-2.2.yml it will only be applied to Rubies 2.2 and lower, not to anyone on 2.5. If these methods are not on Ruby 1.9, what you would want is to enable it in base.yml and then disable this cop in ruby-1.9.yml

[searls]

Wups, I misunderstood something. Reopening because I misread the meaning of ~ in the config earlier and can see now this only changes each_with_index to each.with_index. I'm more amenable to this for your original reasoning that it's both instructive of a handy but lesser-known way of chaining enumerables and doesn't really alter any semantic meaning or user preference.

[danielmorrison]

I want to be clear that I’m only suggesting with_indexand with_object. I have to ~ the others so they don’t start making recommendations once I enable the cop.

I have opinions on the other methods, but not strong enough to add here.

[searls]

I want to be clear that I’m only suggesting with_indexand with_object. I have to ~ the others so they don’t start making recommendations once I enable the cop.

Yes I understand that now. Do you know what the ~ value actually does in this instance? Is it a YAML thing or a RuboCop thing?

[jacobthemyth]

Ruby, at its very heart, embraces the "there's more than one way" philosophy (which is why I personally think it's a terrible first programming language to learn). A few examples that have come up during code reviews in recent memory:

• foo.casecmp?(bar) or foo.downcase == bar.downcase
• hash.each_key or hash.keys.each
• Kernel#Array or Array.try_convert
• foo = hash.fetch(:bar, :baz) or foo = hash[:bar] || :baz
• [:foo] + [:bar] or [:foo].concat([:bar])

Obviously, these aren't all semantically identical expressions, nor are they necessarily the same category of coding style as the suggested change, but my tendency is to reach for different methods when they "feel right" (yeah 🤢 I know). So on a philosophical level, I hesitate to believe that Standard can or should address the category of coding style that includes prescribing specific semantically identical methods. Unfortunately, I'm not sure how to articulate why I think method prescriptions should be excluded while do; end vs { } blocks seems like a worthwhile prescription. It just seems to me that the aesthetics and the conveyance of meaning to the human reader vary from context to context when it comes to which messages to send.

To actually address the original issue, if I understand correctly, the implicit argument (ignoring the specific syntax) is to prefer Enumerator iteration over Enumerable iteration so that the developer doesn't need to have memorized the entire Enumerable API. I disagree, mostly just because I prefer the aesthetics of each_with_index, but since that's a terrible argument, some additional reasons:

• The concept of an Enumerator is much more abstract than the concept of an Enumerable. I tend to avoid the explicit creation of Enumerators so that I don't have to think about what an Enumerator is unless I need to.
• Not all Enumerable methods return an Enumerator when you call them without a block, which means your argument that a developer can just guess at the right syntax is not really true. When they try [].reduce.with_index it will fail. Obviously, that expression doesn't even make sense, so I'm not sure if that's a good counterargument. Also, it seems like you might be arguing that reduce/inject should never be used, preferring each.with_object, but that's not in this PR, and I would take issue with that for other reasons. The point being that I believe familiarity with the Enumerable API is essential and can't be skipped over.
• While each.with_index and each_with_index seem semantically identical, they're actually two completely different code paths in CRuby (https://github.com/ruby/ruby/blob/98e65d9d921ec810bbae2233b80e865e76dd8502/enum.c#L2306, https://github.com/ruby/ruby/blob/trunk/enumerator.c#L569) so they could have divergent performance characteristics or even subtle differences in behavior, so automatically changing a large code base could have unforeseen consequences.

All those reasons are basically just me justifying my feelings though, so at the end of the day I just think Standard shouldn't prescribe "one right way" when it comes to which methods to call.

[searls]

@Iakobos, as always I love your thoughtfulness and self-reflection! I think these are good arguments for being pretty conservative about changing any method calls, even the with_index variants here.

I'd like to give @danielmorrison a chance to mull over this and respond

[danielmorrison]

@searls I updated to move this to base.yml and disable in ruby-1.8.yml.

The ~ is YAML's version of nil and Rubocop calls it out as the correct way to ignore this. The cop is disabled by default in Rubocop. By enabling, we need to use ~.

I'll follow up later on the philosophical issues.

[searls]

Deciding to close this PR. Very much appreciate the sentiment and @danielmorrison's perspective. Feel like in this case because it can't be applied to 100% of cases without potentially confusing folks means it's not appropriate to prescribe. Kudos for more visibility for this feature though. I know I underuse it.