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

Add Enumerable#index_with. #32523

Merged
merged 2 commits into from May 21, 2018
Merged

Conversation

@kaspth
Copy link
Member

@kaspth kaspth commented Apr 10, 2018

In the app I'm working on I've wished that index_by had a buddy that would
assign the hash value instead of the key multiple times.

Enter index_with. Useful when building a hash from a static list of
symbols. Before you'd do:

POST_ATTRIBUTES.map { |attr_name| [ attr_name, public_send(attr_name) ] }.to_h

But now that's a little clearer and faster with:

POST_ATTRIBUTES.index_with { |attr_name| public_send(attr_name) }

It's also useful when you have an enumerable that should be converted to a hash,
but you don't want to muddle the code up with the overhead that it takes to create
that hash. So before, that's:

WEEKDAYS.each_with_object(Hash.new) do |day, intervals|
  intervals[day] = [ Interval.all_day ]
end

And now it's just:

WEEKDAYS.index_with([ Interval.all_day ])

It's also nice to quickly get a hash with either nil, [], or {} as the value.

@kaspth kaspth self-assigned this Apr 10, 2018
@jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented Apr 10, 2018

I've also needed this method on numerous occasions, but I named it index_to. What do you think of that name?

Also, for the last code snippet, I think you meant WEEKDAYS.index_with { [ Interval.all_day ] }, yes?

@albertoalmagro
Copy link
Contributor

@albertoalmagro albertoalmagro commented Apr 13, 2018

I like it, 👍

@khiav223577
Copy link

@khiav223577 khiav223577 commented Apr 23, 2018

Wanna to have this method in ruby too! 👍

@kaspth kaspth force-pushed the enumerable-index-with-extension branch from 8c02446 to a17cb39 May 21, 2018
kaspth added 2 commits May 21, 2018
In the app I'm working on I've wished that index_by had a buddy that would
assign the hash value instead of the key multiple times.

Enter index_with. Useful when building a hash from a static list of
symbols. Before you'd do:

```ruby
POST_ATTRIBUTES.map { |attr_name| [ attr_name, public_send(attr_name) ] }.to_h
```

But now that's a little clearer and faster with:

````ruby
POST_ATTRIBUTES.index_with { |attr_name| public_send(attr_name) }
```

It's also useful when you have an enumerable that should be converted to a hash,
but you don't want to muddle the code up with the overhead that it takes to create
that hash. So before, that's:

```ruby
WEEKDAYS.each_with_object(Hash.new) do |day, intervals|
  intervals[day] = [ Interval.all_day ]
end
```

And now it's just:

```ruby
WEEKDAYS.index_with([ Interval.all_day ])
```

It's also nice to quickly get a hash with either nil, [], or {} as the value.
@kaspth kaspth force-pushed the enumerable-index-with-extension branch from a0c4128 to 429f15f May 21, 2018
@kaspth kaspth merged commit 41147e3 into rails:master May 21, 2018
1 check was pending
@kaspth kaspth deleted the enumerable-index-with-extension branch May 21, 2018
@kaspth
Copy link
Member Author

@kaspth kaspth commented May 21, 2018

I've also needed this method on numerous occasions, but I named it index_to. What do you think of that name?

@jonathanhefner index_to makes me think of our to and from Array/String extensions.

index_to also doesn't suggest its relationship to the existing index_by strongly enough to me. Of course, it's arguable if index_with does that better. But by and with do seem to imply a certain closeness, as well as their respective difference.

Also, for the last code snippet, I think you meant WEEKDAYS.index_with { [ Interval.all_day ] }, yes?

I just noticed I'd forgotten to push that bit. Thanks!

@jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented May 21, 2018

@kaspth Thank you for your response! Even though it's too late to change, I still want to defend my name choice. 😉 The connection between index_by and index_to is like the connection between "a letter written by someone" and "a letter written to someone." There is an implication of an arrow--in the case of a letter, the arrow from the writer to the reader; in the case of a Hash, the arrow from the key to the value.

Also, to me, index_with sounds more like an alias of index_by. For example, I would have guessed ["abc"].index_with(&:upcase) creates the index keys using upcase.

Either way, it's nice to have this in Rails.

@mcary
Copy link

@mcary mcary commented May 31, 2018

I'm with @jonathanhefner: index_with sounds synonymous to index_by. My first impression when I read the sample code is that I expect the block after the "with" to generate the index, not the value.

If this goes to Ruby, I could imagine one of these method names being clearer:

  • index_for
  • map_to
  • with_values
  • indexing
  • indexing_values
  • index_of

index_of and index_for seem to have a clearer relationship to index_with, but index_of in some languages means what Array#index means in Ruby, so that's a potential source of confusion. indexing_values is a bit long. And indexing sounds more declarative than imperative, which might be confusing. Same for index_to (suggested above).

Proc#to_h(enumerable) might be another way of generating an Hash from an enumerable and a proc, but using proc with a block is not as concise as passing a block to an Enumerable method, and maybe really the Enumerable should contain arrays of arguments rather than assuming only one argument...

@vaibhavatul47
Copy link

@vaibhavatul47 vaibhavatul47 commented Jun 1, 2018

I agree with @mcary, that index_with gives an impression that the values from block will be used as keys in hash rather than values in hash. IMO, map_to is a great suggestion.

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this issue Jun 7, 2018
- Clarify executor of `public_send`.
- Do not wrap `Interval.all_day` into [] since
  an array is expected as a returned value.

Related to rails#32523.

[ci skip]
@kaspth
Copy link
Member Author

@kaspth kaspth commented Jul 2, 2018

@jonathanhefner it's only too late once we've shipped Rails 6. At least I'm happy to keep going on this.

I'm still not on board with the _to versions. index_to makes me go: to whom? And I find a pneumonic like "index by key" and "index with value" just as obvious. _with wins because it flows better. index_with_values is closer and clearer, but then I don't mind some conciseness in the vein of select/collect/reject (note the similarity in wording there as well).

I can see how it sounds like a straight alias though.

@reggieb
Copy link

@reggieb reggieb commented Oct 12, 2018

If this was the normal behaviour for array.to_h:

[1,2,3].to_h # => {1 => 1, 2 => 2, 3 => 3}

And the array of pairs behaviour was just a special case:

[[:a, 1], [:b, 2]] # => { a: 1, b: 2}

Then the index_with behaviour could be achieved with:

hash = [1,2,3].to_h
hash.transform_values { |v| v * 2 } # => { 1 => 2, 2 => 4, 3 => 6 }

Which I think is easier to understand. It would also then be just as simple to modify the keys rather than the values:

hash.transform_keys { |k| (k + 96).chr.to_sym } # => {a: 1, b: 2, c: 3}

Therefore perhaps a better solution is:

class Array
  def to_h
    if all? {|i| i.kind_of?(Array) && i.length == 2}
      super
    else
      map {|x| [x,x]}.to_h
    end
  end
end

@saraid
Copy link

@saraid saraid commented Jun 21, 2019

I've always had this named Array#expand in my personal code:

%w(one two three).expand(&:upcase)
=> {"one"=>"ONE", "two"=>"TWO", "three"=>"THREE"}

Admittedly has no relation to any of the other proposals, but I find it far more intuitive than #index_with.

@sj26
Copy link
Contributor

@sj26 sj26 commented Jun 22, 2019

Careful, these are not equivalent:

WEEKDAYS.each_with_object(Hash.new) do |day, intervals|
  intervals[day] = [ Interval.all_day ]
end

And now it's just:

WEEKDAYS.index_with([ Interval.all_day ])

The difference being the first constructs a new array instance for each value of the hash, while the second uses the same array instance for each value. If you mutate this value, like appending new Interval instance to one of the weekday arrays, then they will cause different results. (I've hit this bug before. 😅)

In the first example, if assigned to intervals, and we assume WEEKDAYS is ['Monday', 'Tuesday', ...], and give Interval an arbitrary argument to #new to differentiate, the following will work as expected:

intervals['Monday'] << Interval.new(1)
# intervals['Monday'] == [Interval.all_day, Interval.new(1)]

intervals['Tuesday'] << Interval.new(2)
intervals['Tuesday'] << Interval.new(3)
# intervals['Tuesday'] == [Interval.all_day, Interval.new(2), Interval.new(3)]

But in the second case:

intervals['Monday'] << Interval.new(1)
# intervals['Monday'] == [Interval.all_day, Interval.new(1)]

intervals['Tuesday'] << Interval.new(2)
intervals['Tuesday'] << Interval.new(3)
# intervals['Tuesday'] == [Interval.all_day, Interval.new(1), Interval.new(2), Interval.new(3)]
# but also:
# intervals['Monday'] == [Interval.all_day, Interval.new(1), Interval.new(2), Interval.new(3)]
# and every other value, too.

It's the same difference as:

Hash.new([Interval.all_day])

vs:

Hash.new { [Interval.all_day] }

and can be fixed the same way. It's just a buyer-beware gotcha. :-)

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

Successfully merging this pull request may close these issues.

None yet

10 participants