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
Hashie::Extensions::Persistable #262
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
module Hashie | ||
module Extensions | ||
# Marker module to indicate has a single-argument hash constructor. | ||
module HashInitializer | ||
end | ||
|
||
# Doesn't work... | ||
# module MergeInitializer | ||
# include HashInitializer | ||
# end | ||
end | ||
|
||
class Hash | ||
# Not unless MergeInititalizer is included | ||
end | ||
|
||
class Mash | ||
include Extensions::HashInitializer | ||
end | ||
|
||
class Dash | ||
include Extensions::HashInitializer | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
module Hashie | ||
module Extensions | ||
module Persistable | ||
CANNOT_INCLUDE = 'Peristable can only be used on classes with Hashie::Extensions::HashInitializer' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't do this around Hashie as far as I can see. Maybe just specialize an exception that inherits from StandardError? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is it you don't do? Constants? ArgumentError? Happy to make the change just wasn't really sure what you meant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think just this: class MissingHashInitializer < StandardError; end I would just not include the lengthy explanation, if we do it should be a localizable thing, etcetera. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want text in exceptions, I think I want something evolved like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I think the message is important because a user probably won't know what a MissingHashInitializer means. I can see the value of a localizable system, but I'd rather have that be a separate issue/PR, since there's other errors with messages (e.g. here and here). How about if I do this, and then you can create a separate issue for localization of error messages? class MissingHashInitializer < StandardError
def initialize(msg = 'Peristable can only be used on classes with Hashie::Extensions::HashInitializer', *args)
super
end
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. As long as we do it the same way everywhere. We can leave nice(r) errors for some future implementation. |
||
CANNOT_SAVE = 'Cannot save unless persistable_file is set or the target file is passed as a parameter' | ||
|
||
module ClassMethods | ||
def load(file) | ||
new(YAML.load(File.read(file))).tap do |h| | ||
h.persistable_file = file | ||
end | ||
end | ||
end | ||
|
||
def self.included(base) | ||
# Can only include into classes with a hash initializer | ||
fail ArgumentError, CANNOT_INCLUDE unless | ||
base.include?(Hashie::Extensions::HashInitializer) || | ||
base.include?(Hashie::Extensions::MergeInitializer) | ||
base.extend ClassMethods | ||
super | ||
end | ||
|
||
def save(file = nil) | ||
self.persistable_file = file unless file.nil? | ||
fail ArgumentError, CANNOT_SAVE if persistable_file.nil? | ||
File.write(persistable_file, YAML.dump(self)) | ||
persistable_file | ||
end | ||
|
||
def persistable_file=(file) | ||
@persistable_file = Pathname(file) | ||
end | ||
|
||
attr_reader :persistable_file | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
--- | ||
foo: bar | ||
msg: Hello, world! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
require 'tempfile' | ||
|
||
module Hashie | ||
module Extensions | ||
RSpec.describe Persistable do | ||
shared_examples 'loads YAML' do | test_class | | ||
describe '.load' do | ||
subject(:hash) { test_class.load('spec/fixtures/persistable/hello_world.yaml') } | ||
it 'instantiates a instance from a YAML file' do | ||
expect(hash).to be_a_kind_of ::Hash | ||
expect(hash).to eq( | ||
'foo' => 'bar', | ||
'msg' => 'Hello, world!' | ||
) | ||
end | ||
|
||
it 'remembers the filename' do | ||
expect(hash.persistable_file).to eq(Pathname('spec/fixtures/persistable/hello_world.yaml')) | ||
end | ||
end | ||
end | ||
|
||
shared_examples 'saves YAML' do | test_class | | ||
describe '#save' do | ||
let(:obj) do | ||
test_class.new( | ||
'foo' => 'bar!', | ||
'msg' => 'saved!' | ||
) | ||
end | ||
|
||
it 'raises an error if persistable_file is not set' do | ||
expect { obj.save }.to raise_error(ArgumentError, /cannot save unless persistable_file is set/i) | ||
end | ||
|
||
it 'saves to persistable_file' do | ||
tmpfile = Tempfile.new(['hash', '.yaml']) | ||
obj.persistable_file = tmpfile | ||
expect(obj.save).to eq(Pathname(tmpfile)) | ||
expect(test_class.load(tmpfile)).to eq(obj) | ||
end | ||
|
||
it 'changes and saves to persistable_file (passed as an argument)' do | ||
tmpfile = Tempfile.new(['hash', '.yaml']) | ||
expect(obj.save(tmpfile)).to eq(Pathname(tmpfile)) | ||
expect(obj.persistable_file).to eq(Pathname(tmpfile)) | ||
expect(test_class.load(tmpfile)).to eq(obj) | ||
end | ||
end | ||
|
||
describe '#[]=' do | ||
context 'with autosave enabled' | ||
pending 'autosaves' | ||
context 'without autosave enabled' do | ||
pending 'does not autosave' | ||
end | ||
end | ||
end | ||
|
||
context 'with Hash' do | ||
class MyHash < ::Hash | ||
include Hashie::Extensions::MergeInitializer | ||
include Hashie::Extensions::Persistable | ||
end | ||
|
||
include_examples 'loads YAML', MyHash | ||
include_examples 'saves YAML', MyHash | ||
end | ||
|
||
context 'with Hashie::Hash' do | ||
class MyHashieHash < Hashie::Hash | ||
include Hashie::Extensions::MergeInitializer | ||
include Hashie::Extensions::Persistable | ||
end | ||
|
||
include_examples 'loads YAML', MyHashieHash | ||
include_examples 'saves YAML', MyHashieHash | ||
end | ||
|
||
context 'with Hashie::Mash' do | ||
class MyHashieMash < Hashie::Mash | ||
include Hashie::Extensions::Persistable | ||
end | ||
|
||
include_examples 'loads YAML', MyHashieMash | ||
include_examples 'saves YAML', MyHashieMash | ||
end | ||
|
||
context 'with Hashie::Dash' do | ||
class MyHashieDash < Hashie::Dash | ||
include Hashie::Extensions::Persistable | ||
property 'foo' | ||
property 'msg' | ||
end | ||
|
||
include_examples 'loads YAML', MyHashieDash | ||
include_examples 'saves YAML', MyHashieDash | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I created
Hashie::Extensions::HashInitializer
that's just a marker module, because I didn't have a way to tell what classes could be initialized from a Hash. That's the behavior forMash
andDash
, but that's not how the initializer works (by default) for::Hash
orHashie::Hash
. It is the behavior if you mixinHashie::Extensions::MergeInitializer
, though. I'm open to other ideas, though, perhaps a.from_hash
method instead of a marker module?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK.