Skip to content

Conversation

@Jack12816
Copy link

First things first: thank you for this awesome gem and your work! ❤️

- What is it good for

I accidentally hit funny errors when wrapping a ros object in a new row object like this: RecursiveOpenStruct.new(RecursiveOpenStruct.new({a: true})). Then I digged into this and found different behavior for OpenStruct.

Show more examples
# Reference how OpenStruct works:

» OpenStruct.new({a: true})
# => #<OpenStruct a=true>
» OpenStruct.new(OpenStruct.new({a: true}))
# => #<OpenStruct a=true>
» OpenStruct.new(OpenStruct.new({a: true})).a
# => true
» a = OpenStruct.new({a: true}); b = OpenStruct.new(a); a.object_id == b.object_id
# => false

# Before the patch:

» RecursiveOpenStruct.new({a: true})
# => #<RecursiveOpenStruct a=true>
» RecursiveOpenStruct.new(RecursiveOpenStruct.new({a: true}))
# => #<RecursiveOpenStruct:0x00007f84ebefe120 ...>
» RecursiveOpenStruct.new(RecursiveOpenStruct.new({a: true})).a
# => NoMethodError: undefined method `has_key?' for #<RecursiveOpenStruct a=true> (NoMethodError)
#                   from gems/recursive-open-struct-2.0.0/lib/recursive_open_struct.rb:205
#                   in 'RecursiveOpenStruct#_get_key_from_table_'
» a = RecursiveOpenStruct.new({a: true}); b = RecursiveOpenStruct.new(a); a.object_id == b.object_id
# => false

# After the patch:

» RecursiveOpenStruct.new({a: true})
# => #<RecursiveOpenStruct a=true>
» RecursiveOpenStruct.new(RecursiveOpenStruct.new({a: true}))
# => #<RecursiveOpenStruct a=true>
» RecursiveOpenStruct.new(RecursiveOpenStruct.new({a: true})).a
# => true
» a = RecursiveOpenStruct.new({a: true}); b = RecursiveOpenStruct.new(a); a.object_id == b.object_id
# => false

- What I did

I just added a safety guard on the class constructor to unpack ros/os (or children of them) objects first. This leads to deep copies.

- A picture of a cute animal (not mandatory but encouraged)
image

@aetherknight
Copy link
Owner

Tests are failing due to changes out side of this pull request. jruby-head is failing, I think because I'm pinning to an older version of the setup-ruby action. I'll get a fix for that added soon.

@aetherknight
Copy link
Owner

@Jack12816 when you get a chance, can you merge or rebase to my current main branch? It contains fixes for the CI test runs.

@Jack12816
Copy link
Author

Will do in a couple hours :)

Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>
@Jack12816
Copy link
Author

@aetherknight I rebased against upstream main.

@Jack12816
Copy link
Author

I'll fix the broken specs tomorrow. I didn't tested the older rubies locally - sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants