Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix syck cannot dump DirtyMinded Strings #50

Merged
merged 3 commits into from

2 participants

@mbj
Owner
mbj commented

The fix excludes Strings from be extended by the Hooker.

This affects ruby-1.8.7 and ruby-1.9.2 (when using syck), both master and dm-1.2.0.

Markus Schirp added some commits
Markus Schirp Fix syck cannot dump DirtyMinded Strings
The fix excludes Strings from be extended by the Hooker.
68562ae
Markus Schirp Removing my debug statements, sorry. 8d070c9
@mbj
Owner

I always forget to check git diff HEAD since I'm using github :D

spec/integration/yaml_spec.rb
@@ -64,6 +64,24 @@
end
end
end
+
+ describe 'with invetions as a string' do
@emmanuel Owner
emmanuel added a note

nitpicking a little: 'invetions' is a typo of 'inventions'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/integration/yaml_spec.rb
@@ -64,6 +64,24 @@
end
end
end
+
+ describe 'with invetions as a string' do
+ before :all do
+ object = "Foo and Bar" #.freeze
+ @resource.inventions = object
+ end
+
+ describe 'when dumpoed and loaded again' do
@emmanuel Owner
emmanuel added a note

nitpicking again: 'dumpoed' is a typo of 'dumped'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/integration/yaml_spec.rb
@@ -64,6 +64,24 @@
end
end
end
+
+ describe 'with invetions as a string' do
+ before :all do
+ object = "Foo and Bar" #.freeze
+ @resource.inventions = object
+ end
+
+ describe 'when dumpoed and loaded again' do
+ before :all do
+ @resource.save.should be(true)
+ @resource.reload
+ end
+
+ it 'has correct invetions' do
@emmanuel Owner
emmanuel added a note

typo here as well

@mbj Owner
mbj added a note

When im typing a word wrong, I misspell other instances also for consistency... will fix this shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@emmanuel emmanuel commented on the diff
lib/dm-types/support/dirty_minder.rb
@@ -148,7 +148,10 @@ def track(resource, property)
# Catch any direct assignment (#set), and any Resource#reload (set!).
def set!(resource, value)
- hook_value(resource, value) unless value.kind_of? Hooker
+ # Do not extend non observed value classes
+ if Hooker::MUTATION_METHODS.keys.detect { |klass| value.kind_of?(klass) }
@emmanuel Owner
emmanuel added a note

It seems like there are only a few types that could have String, Hash, or Array values: YAML and JSON are the only two I can think of.

It would be nice to not do this iteration on Property#set! calls for Property instances that don't have to worry about this, but I'm not sure how to manage that.

@mbj Owner
mbj added a note

We could do MUTATION_METHODS.key?(value.class), but this will not catch subclasses.

This iteration will not be done on all Property instances, since the DirtyMinder is only included in DataMapper::Property::{Yaml,Json} ? So I do not get your point here.

@emmanuel Owner
emmanuel added a note

Oh, sorry. I forgot the bigger context. I thought that DirtyMinder got included in more Property subclasses, and I didn't check before commenting.

Thanks for the correction :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Markus Schirp Fix those typos 3c795ed
@emmanuel
Owner

@mbj—sorry for losing track of this, Github doesn't ping me when commits are added to a pull request that I'm participating in, so I didn't notice when you fixed the typos. Merging!

@emmanuel emmanuel merged commit 08966f3 into datamapper:master
@mbj
Owner

So I'll to @emmanuel spamming in the future :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 7, 2011
  1. Fix syck cannot dump DirtyMinded Strings

    Markus Schirp authored
    The fix excludes Strings from be extended by the Hooker.
  2. Removing my debug statements, sorry.

    Markus Schirp authored
  3. Fix those typos

    Markus Schirp authored
This page is out of date. Refresh to see the latest.
View
5 lib/dm-types/support/dirty_minder.rb
@@ -148,7 +148,10 @@ def track(resource, property)
# Catch any direct assignment (#set), and any Resource#reload (set!).
def set!(resource, value)
- hook_value(resource, value) unless value.kind_of? Hooker
+ # Do not extend non observed value classes
+ if Hooker::MUTATION_METHODS.keys.detect { |klass| value.kind_of?(klass) }
@emmanuel Owner
emmanuel added a note

It seems like there are only a few types that could have String, Hash, or Array values: YAML and JSON are the only two I can think of.

It would be nice to not do this iteration on Property#set! calls for Property instances that don't have to worry about this, but I'm not sure how to manage that.

@mbj Owner
mbj added a note

We could do MUTATION_METHODS.key?(value.class), but this will not catch subclasses.

This iteration will not be done on all Property instances, since the DirtyMinder is only included in DataMapper::Property::{Yaml,Json} ? So I do not get your point here.

@emmanuel Owner
emmanuel added a note

Oh, sorry. I forgot the bigger context. I thought that DirtyMinder got included in more Property subclasses, and I didn't check before commenting.

Thanks for the correction :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ hook_value(resource, value) unless value.kind_of? Hooker
+ end
super
end
View
18 spec/integration/yaml_spec.rb
@@ -64,6 +64,24 @@
end
end
end
+
+ describe 'with inventions as a string' do
+ before :all do
+ object = "Foo and Bar" #.freeze
+ @resource.inventions = object
+ end
+
+ describe 'when dumped and loaded again' do
+ before :all do
+ @resource.save.should be(true)
+ @resource.reload
+ end
+
+ it 'has correct inventions' do
+ @resource.inventions.should == 'Foo and Bar'
+ end
+ end
+ end
end
end
end
View
64 spec/unit/dirty_minder_spec.rb
@@ -0,0 +1,64 @@
+require 'spec_helper'
+require 'dm-types/support/dirty_minder'
+
+describe DataMapper::Property::DirtyMinder,'set!' do
+
+ let(:property_class) do
+ Class.new(DataMapper::Property::Object) do
+ include DataMapper::Property::DirtyMinder
+ end
+ end
+
+ let(:model) do
+ property_class = self.property_class
+ Class.new do
+ include DataMapper::Resource
+ property :id,DataMapper::Property::Serial
+ property :name,property_class
+
+ def self.name; 'FredsClass'; end
+ end
+ end
+
+ let(:resource) { model.new }
+
+ let(:object) { model.properties[:name] }
+
+ subject { object.set!(resource,value) }
+
+ shared_examples_for 'a non hooked value' do
+ it 'should not extend value with hook' do
+ value.should_not be_kind_of(DataMapper::Property::DirtyMinder::Hooker)
+ end
+ end
+
+ shared_examples_for 'a hooked value' do
+ it 'should extend value with hook' do
+ value.should be_kind_of(DataMapper::Property::DirtyMinder::Hooker)
+ end
+ end
+
+ before do
+ subject
+ end
+
+ context 'when setting nil' do
+ let(:value) { nil }
+ it_should_behave_like 'a non hooked value'
+ end
+
+ context 'when setting a String' do
+ let(:value) { "The fred" }
+ it_should_behave_like 'a non hooked value'
+ end
+
+ context 'when setting an Array' do
+ let(:value) { ["The fred"] }
+ it_should_behave_like 'a hooked value'
+ end
+
+ context 'when setting a Hash' do
+ let(:value) { {"The" => "fred"} }
+ it_should_behave_like 'a hooked value'
+ end
+end
Something went wrong with that request. Please try again.