Rails core - Jun 2010 - [PATCH] ActiveSupport OrderedHash Doesn't Support Passing Block to merge

Hi all,

On versions of Ruby prior to 1.9 (where Hashes'' keys have no set
order), ActiveSupport::OrderedHash implements its own merge and merge!
methods. These implementations differ to the standard library however
in that they do not support passing a block to merge (c.f.
http://ruby-doc.org/core/classes/Hash.html#M002880 for the standard
method signature).

I have posted a patch including tests at
https://rails.lighthouseapp.com/projects/8994/tickets/4838-patch-add-block-support-to-activesupport-orderedhash-merge
to correct this missing functionality. The implementation also closely
matches that of Rubinius'' (c.f.
http://github.com/evanphx/rubinius/blob/master/kernel/common/hash.rb#L429-447
)

Any feedback would be greatly appreciated.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

On Sat, Jun 12, 2010 at 4:23 PM, mudge <mudge@mudge.name> wrote:
> On versions of Ruby prior to 1.9 (where Hashes'' keys have no set
> order), ActiveSupport::OrderedHash implements its own merge and merge!
> methods. These implementations differ to the standard library however
> in that they do not support passing a block to merge (c.f.
> http://ruby-doc.org/core/classes/Hash.html#M002880 for the standard
> method signature).
>
> I have posted a patch including tests at
>
https://rails.lighthouseapp.com/projects/8994/tickets/4838-patch-add-block-support-to-activesupport-orderedhash-merge
> to correct this missing functionality. The implementation also closely
> matches that of Rubinius'' (c.f.
http://github.com/evanphx/rubinius/blob/master/kernel/common/hash.rb#L429-447
> )
Excellent, applied

    http://github.com/rails/rails/compare/a087bc8...9183eae

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

Just realized the patch is not compatible with 1.9, since in 1.9 only
existing keys are merged (that''s the key? key test in Rubinius). Will
revise it myself.

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

On Sun, Jun 13, 2010 at 4:42 AM, Xavier Noria <fxn@hashref.com> wrote:
> Just realized the patch is not compatible with 1.9, since in 1.9 only
> existing keys are merged (that''s the key? key test in Rubinius).
Will
> revise it myself.
Rather, yield is called only for existing keys. This revisions seems
to be correct:

http://github.com/rails/rails/commit/36143d26cb841210b5f22aff4ed9c093a0554a1a

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

I''ve done a tiny improvement to this implementation.
Please Xavier review it ;).
http://github.com/spastorino/rails/commit/63b43575765c7d4042c23ac3ddb1213c1968b5a3

On Sun, Jun 13, 2010 at 1:42 AM, Xavier Noria <fxn@hashref.com>
wrote:
> On Sun, Jun 13, 2010 at 4:42 AM, Xavier Noria <fxn@hashref.com>
wrote:
>
>> Just realized the patch is not compatible with 1.9, since in 1.9 only
>> existing keys are merged (that''s the key? key test in
Rubinius). Will
>> revise it myself.
>
> Rather, yield is called only for existing keys. This revisions seems
> to be correct:
>
>
http://github.com/rails/rails/commit/36143d26cb841210b5f22aff4ed9c093a0554a1a
>
> --
> You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
> To post to this group, send email to rubyonrails-core@googlegroups.com.
> To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.
>
>
-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.

On Sun, Jun 13, 2010 at 7:14 AM, Santiago Pastorino
<santiago@wyeworks.com> wrote:
> I''ve done a tiny improvement to this implementation.
> Please Xavier review it ;).
>
http://github.com/spastorino/rails/commit/63b43575765c7d4042c23ac3ddb1213c1968b5a3
Good, it is up
http://github.com/rails/rails/commit/6d19a4a664914e908e75cfe90a0507cc9f53d1cd

Thanks!

-- 
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com.
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en.