This project is archived and is in readonly mode.

#490 ✓resolved
Lawrence Pit

Nested with_options merge hash values

Reported by Lawrence Pit | June 26th, 2008 @ 01:07 AM

Consider this:

map.with_options :conditions => { :subdomain => /^www$/ } do |m|

m.with_options :conditions => { :method => :get } do |home|

home.main_home '/'

  1. etc.

end

  1. etc.

end

I expected that hitting http://foo.mydomain.com would not match home.main_home, but it does. This is because the with_options method overwrites the value for :conditions, losing the condition for subdomain.

Attached is a patch including tests that will merge hash values within options.

In the above example this would effectively result in:

map.main_home "/", :conditions => { :subdomain => /^www$/, :method => :get }

Comments and changes to this ticket

  • Lawrence Pit

    Lawrence Pit June 27th, 2008 @ 01:03 AM

    • Tag changed from “activesupport” to “activesupport, core_ext, patch, tests”

    Again, hopefully nicely formatted this time: 

    map.with_options :conditions => { :subdomain => /^www$/ } do |m|
  m.with_options :conditions => { :method => :get } do |home|

    home.root '/'
  end
end

    should equal: 

    map.root '/', :conditions => { :subdomain => /^www$/, :method => :get }
  • Pratik

    Pratik July 12th, 2008 @ 04:50 PM

    • Assigned user set to “Pratik”

    Hey Lawrence,

    Looks nice. I wonder if we should rather add Hash#deep_merge method and simply use it here. Thoughts ?

  • Lawrence Pit

    Lawrence Pit July 14th, 2008 @ 03:01 AM

    Attached version with deep_merge.

    The code of deep_merge is a bit ugly though... You need to test for :to_hash for it to work with with_options, because OrderedHash is an Array, not a Hash. Nicer would of course be if you only needed to test for is_a?(Hash).

    Secondly, calling is_a?(Hash) doesn't work within deep_merge. hash/conversions.rb shows the same issue, it comments:

    1. something weird with classes not matching here. maybe singleton methods breaking is_a?

    hence the use of: val.class.to_s == 'Hash'.

  • Lawrence Pit

    Lawrence Pit July 14th, 2008 @ 03:17 AM

    • no changes were found...
  • Lawrence Pit

    Lawrence Pit July 14th, 2008 @ 03:17 AM

    just thinking, maybe we should add this method to OrderedHash? : 

       def is_a?(o)
     o == Hash
   end

    Last patch implements this. Simplifies deep_merge.

  • Repository

    Repository July 17th, 2008 @ 02:02 AM

    • State changed from “new” to “resolved”

    (from [40dbebba28bfa1c55737da7354542c3bdca4e1a1]) Allow deep merging of hash values for nested with_options. [#490 state:resolved]

    Signed-off-by: Pratik Naik

    http://github.com/rails/rails/co...

  • Pratik

    Pratik July 17th, 2008 @ 02:20 AM

    • State changed from “resolved” to “new”

    Intentionally committed version without OrderedHash#is_a? as OrderedHash doesn't really behave like a hash ( lots of missing features like merge/update/etc. )

  • Pratik

    Pratik July 17th, 2008 @ 02:20 AM

    • State changed from “new” to “resolved”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages