@deepj the bang version can be implemented, as long as they don't change the type of the values nor keys.

Would you prefer to implement those in this PR as well?

@bcardiff I meant the bang versions cannot work in the same way (including type casting)

Yes, I can add them including a notice they don't work with type casting.

I tried to implement transform_keys! the following way

class Hash
  def transform_keys!(&block  : K -> K)
    each do |key, value|
      self[yield(key)] = value
      delete(key)
    end
  end
end

h = {'a' => 1, 'b' => 2}
h.transform_keys!(&.upcase)
puts h

But h returns an empty hash and I don't know why. Crystal 0.22.0

@deepj it's because the hash in changed inside the iterator.

For example if you swap delete and self[..] = you will end in an infinite loop.

I think we will need to duplicate the keys. Unless we go down with something that deals with the hash internals for reallocating the entries. So something as follow could work:

class Hash
  def transform_keys!(&block  : K -> K)
    each.to_a.each do |(key, value)|
      delete(key)
      self[yield(key)] = value
    end
  end
end

h = {'a' => 1, 'b' => 2}
h.transform_keys!(&.upcase)
puts h

Note that deleting the key should happen before inserting. Just in case the key transformation did nothing for that key.

But, now i am thinking of the corner cases:

h = {'a' => 1, 'b' => 2}
h.transform_keys!(&.succ)
puts h

The above change leads of {'c' => 2}

So we would need something like

class Hash
  def transform_keys!(&block  : K -> K)
    pairs = each.to_a
    clear
    pairs.each do |(key, value)|
      self[yield(key)] = value
    end
  end
end

which is essentially h = h.transform_keys(&.succ).

So, unless we really do it in-place there is not real value for bang of keys. Let's leave that out.

But the transform_values! could be there. Doing something efficient for this overload seems straight forward. Check Hash#each.

This looks like map for keys/values on hash. Why not call it map_keys/map_values?

@RX14 One reason would be familiarity, since there's already Hash#transform_keys in Rails.

I'm all for map_keys! & map_values! they're the perfect names IMO.

The implementation needs to use internals I think: https://carc.in/#/r/1zwm

class Hash
  def map_keys!
    current = @first
    while current
      current.key = yield current.key
      current = current.fore
    end
  end

  private class Entry
    setter key : K # key has a getter only, adding a setter here
  end
end

h = {'a' => 1, 'b' => 2}
h.map_keys!(&.succ)
puts h # => {'b' => 1, 'c' => 2}

This impl has no copy (well maybe when copying from the block result to the Entry, but I'm not sure..), so it should be the fastest and the safest (no allocations or anything that could fail)

What if two keys would be mapped to the same value? It should raise, not create inconsistent UB'ish state imho. https://carc.in/#/r/200a

I thought this was merged, @deepj do you want to continue working on it?

If we can't find a consensus for mutating methods, at least it would be great to have this for non-mutating ones.

@bew Hi, I totally forgot on this PR. But my conclusion is, the best things here is keeping only #transform_keys and #transform_values.

What do you think @bcardiff @RX14

I think you can also keep transform_values! too, that can be useful at times.

@bew So, I left #transform_keys, #transform_values, #transform_values!

@bcardiff Can be this reviewed again, please? Is something else what I can do here and push things forward?

LGTM, can this have some reviews?