8000 Add merge functionality by davich · Pull Request #44 · aetherknight/recursive-open-struct · GitHub
[go: up one dir, main page]
Skip to content

Add merge functionality #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add merge functionality #44

wants to merge 1 commit into from

Conversation

davich
Copy link
@davich davich commented May 16, 2016

Allows the merging of a hash or another RecursiveOpenStruct

@davich
Copy link
Author
davich commented May 17, 2016

There is a failing test on ruby-head, but I don't think it's from my change... It seems to be failing on master, too.

@aetherknight
Copy link
Owner

This causes another attribute name to become reserved and unusable (eg, if the original hash has a "merge" attribute, you won't be able to access it normally on the ROS. It also makes the ROS more Hash-like than OpenStruct-like.

Is there some reason why ros.merge(some_h) is preferable to something likeros.class.new(ros.to_h.merge(some_h))?

@davich
Copy link
Author
davich commented Jan 25, 2017

The merge method makes the code look nicer. But you're right, it does create another reserved word which isn't great.
It's your call. Feel free to close this PR if you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@davich @aetherknight
0