aetherknight / recursive-open-struct

OpenStruct subclass that returns nested hash attributes as RecursiveOpenStructs
Other
276 stars 54 forks source link

Implemented lazy attribute creation in pair with Ruby's post version … #39

Closed Kris-LIBIS closed 8 years ago

Kris-LIBIS commented 8 years ago

…2.2.3 OpenStruct.

Accessor methods are now only created when needed. See pull requests 1033 and 1037 on Ruby's githug for more information on the upstream change.

This fixes #38.

aetherknight commented 8 years ago

How does the OpenStruct in 2.2.3dev handle respond_to? for lazily defined methods vs its older behavior? Does it act like the methods exist, or will it return false until it defines a given method?

I will take a closer look at the PR tonight or this weekend, but it would probably be a good idea to get Travis to run against ruby-head as well. Travis' ruby 2.2 support looks like it uses 2.2.0 instead of the latest stable 2.2.

Kris-LIBIS commented 8 years ago

In 2.2.3 and earlier respond_to? would return true since all methods are defined as soon as the key is present in the table hash. Lazy definition of accessor methods post-2.2.3 would mean that it could return false if not accessed before. To overcome this, the repsond_to_missing? has been added to post 2.2.3 OpenStruct. I have copied this behaviour in my patch.

BTW: there already exist a couple of tests in open_struct_behavior_spec.rb that check exactly that (#respond? and #methods tests).

TL;DR; It behaves the same.

aetherknight commented 8 years ago

Sounds good. I will look at getting it merged soon.

On Thu, Oct 29, 2015 at 1:51 AM, Kris Dekeyser notifications@github.com wrote:

In 2.2.3 and earlier repond_to? would return true since all methods are defined as soon as the key is present in the table hash. Lazy definition of accessor methods post-2.2.3 would mean that it could return false if not accessed before. To overcome this, the repsond_to_missing? has been added to post 2.2.3 OpenStruct. I have copied this behaviour in my patch.

BTW: there already exist a couple of tests in open_struct_behavior_spec.rb that check exactly that (#respond? and #methods tests).

TL;DR; It behaves the same.

— Reply to this email directly or view it on GitHub https://github.com/aetherknight/recursive-open-struct/pull/39#issuecomment-152114858 .

jrafanie commented 8 years ago

@aetherknight Thanks for working on this gem! 2.3.0 is supposed to be released in 2 weeks, so I tested @Kris-LIBIS's work in this PR against:

It's good on all of those versions.

In case others see this against master...

master is currently breaking on 2.3.0-preview2 due to the referenced issue. It fails with:

NameError or NoMethodError exceptions like these:

  1) RecursiveOpenStruct indifferent access overwriting values keeps original keys after resetting value
     Failure/Error: recursive.foo.first[:bar].first[:foo] = :foo

     NoMethodError:
       undefined method `first' for nil:NilClass
     # ./spec/recursive_open_struct/indifferent_access_spec.rb:141:in `block (5 levels) in <top (required)>'

  2) RecursiveOpenStruct OpenStruct 2.0 methods delete_field removes the value
     Failure/Error: singleton_class.__send__(:remove_method, sym, "#{sym}=")

     NameError:
       method `foo' not defined in #<Class:#<RecursiveOpenStruct:0x007ff6ac40d440>>
     # ./lib/recursive_open_struct.rb:86:in `remove_method'
     # ./lib/recursive_open_struct.rb:86:in `delete_field'
     # ./spec/recursive_open_struct/ostruct_2_0_0_spec.rb:26:in `block (4 levels) in <top (required)>'
aetherknight commented 8 years ago

@jrafanie I just pushed a 1.0.0 release of ROS out which incorporates the changes. The main API breaking change has to do with #34 and #40 . Let me know if you encounter any regressions.

The travis builds are claiming to fail, but that is because I added 2.3.0 to the .travis.yml file to automatically support it once it is officially released. The ruby-head tests are passing, however.

jrafanie commented 8 years ago

@aetherknight Nice, thank you! I'll try it out and report any problems.