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

Add MethodOverridingInitializer extension #455

Closed
wants to merge 3 commits into from
Closed

Add MethodOverridingInitializer extension #455

wants to merge 3 commits into from

Conversation

lnestor
Copy link
Contributor

@lnestor lnestor commented Aug 2, 2018

Describe the change

This PR adds a new extension - MethodOverridingInitializer. This extension allows existing hash methods to be overridden if a hash passed in to the constructor contains such keys. For example:

class OverridingHash < Hash
  include Hashie::Extensions::MethodOverridingInitializer
end

hash = OverridingHash.new(zip: 'a-dee-doo-dah')
hash.zip #=> 'a-dee-doo-dah'
hash.__zip #=> [[[zip a-dee-doo-dah]]]

Issue

References #338

Design Decisions

I made this its own extension in case people want to restrict which methods can be overwritten. Using this extension only, they can pass in a hash of only the methods they want overwritten and prevent overwriting other methods.

Possible Alternatives

This could be included in the extension MethodAccessWithOverride instead of being it's own extension.

@lnestor lnestor changed the title WIP Add MethodOverridingInitializer extension Add MethodOverridingInitializer extension Aug 3, 2018
Copy link
Contributor Author

@lnestor lnestor left a comment

Choose a reason for hiding this comment

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

Some thoughts for discussion if need be.

# h = MyHash.new(zip: 'a-dee-doo-dah')
# h.zip # => 'a-dee-doo-dah'
# h.__zip # => [[['zip', 'a-dee-doo-dah']]]
module MethodOverridingInitializer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this its own extension in case people want to restrict which methods can be overwritten. Using this extension only, they can pass in a hash of only the methods they want overwritten and prevent overwriting other methods.

end

context 'when the original hash has double keys' do
let(:original_hash) { { zip: 'a-dee-doo-dah', zip: 'a-dee-day' } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes a warning to be shown when running the specs. If that's an issue it can easily be removed. I added this spec because the already_overridden? isn't needed in this extension, so I was testing the case of duplicate keys.

@michaelherold
Copy link
Member

Great idea! A couple things I would like to see:

  • A reduction of the duplicated code. Since the redefine_method is the same as in the MethodOverridingWriter, it should be extracted into a helper module so we don't have it defined in multiple places.
  • We could ship this in a minor revision (3.6) as a new extension, but we shouldn't change the way that MethodAccessWithOverride works without a bump to 4.0. I think we could add a log message to the included hook for MethodAccessWithOverride if we want to make it a default in 4.0.

One additional thing that I'm thinking about here is that this somewhat replicates a subset of the behavior of MergeInitializer. I wonder if there is a clean way we can make it as an extension to MergeInitializer. I think we would have to add some sort of hook method that gets called for each key in MergeInitializer.

What are your thoughts about an approach like that?

@lnestor
Copy link
Contributor Author

lnestor commented Aug 3, 2018

I like that idea. We could maybe make it an optional named argument. So, for a normal hash we would do something like MyHash.new(initial_hash, override: true), and for a clash you could do MyClash.new(initial_hash, parent, override: true). Then if you don't want to override, we keep the original way the method was called with MyHash.new(initial_hash).

Although MergeInitializer doesn't relate to method access of properties, so that is another thing to consider. If a person passes in the override: true argument to the constructor, that will create access methods, which is more of the spirit of MethodAccess. This new extension resides somewhere in the middle of MergeInitializer and MethodAccess.

@dblock
Copy link
Member

dblock commented Aug 3, 2018

Neat

@lnestor
Copy link
Contributor Author

lnestor commented Aug 8, 2018

After implementing it both ways, I think it makes more sense to keep this under a MethodOverridingInitializer module. I don't really see this being used without MethodReader, but I can see people wanting to use MergeInitializer without overriding the methods.

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.

This is looking good. I'll do the version bumping in the release commit, so I'm going to pop that off this branch, take the necessary changes from it, and merge outside of Github. You'll still get credit!

Thanks for the contribution.

michaelherold added a commit that referenced this pull request Aug 12, 2018
Add MethodOverridingInitializer extension
@michaelherold
Copy link
Member

michaelherold commented Aug 12, 2018

Merged via cabb3e7

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 23, 2018
## [3.6.0] - 2018-08-13

[3.6.0]: hashie/hashie@v3.5.7...v3.6.0

### Added

* [#455](hashie/hashie#455): Allow overriding methods when passing in a hash - [@lnestor](https://github.com/lnestor).

### Fixed

* [#435](hashie/hashie#435): Mash `default_proc`s are now propagated down to nested sub-Hashes - [@michaelherold](https://github.com/michaelherold).
* [#436](hashie/hashie#436): Ensure that `Hashie::Extensions::IndifferentAccess` injects itself after a non-destructive merge - [@michaelherold](https://github.com/michaelherold).
* [#437](hashie/hashie#437): Allow codependent properties to be set on Dash - [@michaelherold](https://github.com/michaelherold).
* [#438](hashie/hashie#438): Fix: `NameError (uninitialized constant Hashie::Extensions::Parsers::YamlErbParser::Pathname)` in `Hashie::Mash.load` - [@onk](https://github.com/onk).
* [#457](hashie/hashie#457): Fix `Trash` to allow it to copy properties from other properties - [@michaelherold](https://github.com/michaelherold).

### Miscellaneous

* [#433](hashie/hashie#433): Update Rubocop to the most recent version - [@michaelherold](https://github.com/michaelherold).
* [#434](hashie/hashie#434): Add documentation around Mash sub-Hashes - [@michaelherold](https://github.com/michaelherold).
* [#439](hashie/hashie#439): Add an integration spec for Elasticsearch - [@michaelherold](https://github.com/michaelherold).
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

3 participants