Skip to content
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.

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

Deep Merge Initializer Extension #382

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sazor
Copy link
Contributor

@sazor sazor commented Nov 11, 2016

Idea of this extension came from #347 .

@dblock
Copy link
Member

dblock commented Nov 12, 2016

I like it. You just need to fix the build (rubocop -a, etc).

Copy link
Member

@michaelherold michaelherold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple changes to the change log and readme. I also have a thought experiment with the class_eval for initialize. Is that better or would a direct initialize method be better?

@@ -13,7 +13,7 @@ scheme are considered to be bugs.
### Added

* [#381](https://github.com/intridea/hashie/pull/381): Add a logging layer that lets us report potential issues to our users. As the first logged issue, report when a `Hashie::Mash` is attempting to overwrite a built-in method, since that is one of our number one questions - [@michaelherold](https://github.com/michaelherold).
* Your contribution here.
* [#382](https://github.com/intridea/hashie/pull/382): `DeepMergeInitializer` extension - [@sazor](https://github.com/sazor).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure to restore the "Your contribution here." line.

@@ -206,6 +206,10 @@ Hashie.stringify_keys hash # => Returns a copy of hash with keys stringified.

The MergeInitializer extension simply makes it possible to initialize a Hash subclass with another Hash, giving you a quick short-hand.

### DeepMergeInitializer

The DeepMergeInitializer acts the same as MergeInitializer but also cast all nested hashes to extendable class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please place the class names in backticks.

#
module DeepMergeInitializer
def self.included(base)
base.class_eval do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought experiment: is doing class_eval the right thing here? Would it be better to have initialize be an instance method that gets included?


it 'creates nested hash with the same type as parent hash' do
s = subject.new({ a: :b, hash: { c: :d } })[:hash]
pp s.class

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pp ?

@zealot128
Copy link

Great extension! just needed it today but found this PR is open for 3 years. @sazor to you want to pick it up? Otherwise I could try to fix the issues and open a new PR.

@sazor
Copy link
Contributor Author

sazor commented Oct 24, 2019

I could fix it on this week. However, if you need it today or tomorrow then it's probably better for you to pick up.

@zealot128
Copy link

@sazor no pressure :D i've just pulled in your fork at the moment :)

@mculp
Copy link

mculp commented Jan 30, 2023

I bet this guy's almost done with this one!

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.

None yet

6 participants