Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Private callback methods and refactoring #40

Closed
wants to merge 3 commits into from

3 participants

@cordawyn

Greetings! (and sorry for the numerous edits of this pull request - there were some "technical" problems).
Here's a small update from a fan of spira. I believe that the hook (callback) methods generally should be made private (and thus provide for a good API design), but your current implementation calls them only if they are public. I've fixed that and wrote a spec to detect that "bug".

Also added a couple of refactoring commits - while generally making the code more readable (IMHO) and DRY, they were initially aimed at fixing a bug, which was surfaced by abrisse's introduction of Spira::Types::Data property type. Since I cannot submit a spec for it without merging abrisse's pull request, here's a way to detect it until the merge occurs:

class Person
include Spira::Resource
base_uri "http://example.org/people"
property :birthday, :predicate => FOAF.birthday, :type => Date
end
bob = Person.new("bob")
bob.destroy!

The exception is raised at this point, because :birthday property is nil, but Spira::Resource#repository_for_attributes attempts to instantiate it, ultimately calling Date.new(nil).

@abrisse

+1 for this pull. I have done similar modification to repository_for_attributes to avoid the raise you point out but only on my local dev branch.

Hope this pull will be included soon in the official spira branch.

@bendiken
Owner

This project has moved to ruby-rdf/spira. If this pull request is still relevant, please rebase and resubmit it over there.

@bendiken bendiken closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 89 additions and 29 deletions.
  1. +35 −28 lib/spira/resource/instance_methods.rb
  2. +54 −1 spec/hooks.spec
View
63 lib/spira/resource/instance_methods.rb
@@ -139,7 +139,7 @@ def _destroy_attributes(attributes, opts = {})
# @object.destroy!(:completely)
# @return [true, false] Whether or not the destroy was successful
def destroy!(what = nil)
- before_destroy if self.respond_to?(:before_destroy)
+ before_destroy if self.respond_to?(:before_destroy, true)
result = case what
when nil
_destroy_attributes(attributes, :destroy_type => true) != nil
@@ -150,7 +150,7 @@ def destroy!(what = nil)
when :completely
destroy!(:subject) && destroy!(:object)
end
- after_destroy if self.respond_to?(:after_destroy) if result
+ after_destroy if self.respond_to?(:after_destroy, true) if result
result
end
@@ -159,9 +159,9 @@ def destroy!(what = nil)
#
# @return [self] self
def save!
- existed = (self.respond_to?(:before_create) || self.respond_to?(:after_create)) && !self.type.nil? && exists?
- before_create if self.respond_to?(:before_create) && !self.type.nil? && !existed
- before_save if self.respond_to?(:before_save)
+ existed = (self.respond_to?(:before_create, true) || self.respond_to?(:after_create, true)) && !self.type.nil? && exists?
+ before_create if self.respond_to?(:before_create, true) && !self.type.nil? && !existed
+ before_save if self.respond_to?(:before_save, true)
# we use the non-raising validate and check it to make a slightly different error message. worth it?...
case validate
when true
@@ -169,8 +169,8 @@ def save!
when false
raise(ValidationError, "Could not save #{self.inspect} due to validation errors: " + errors.each.join(';'))
end
- after_create if self.respond_to?(:after_create) && !self.type.nil? && !existed
- after_save if self.respond_to?(:after_save)
+ after_create if self.respond_to?(:after_create, true) && !self.type.nil? && !existed
+ after_save if self.respond_to?(:after_save, true)
self
end
@@ -192,7 +192,7 @@ def update(properties)
properties.each do |property, value|
attribute_set(property, value)
end
- after_update if self.respond_to?(:after_update)
+ after_update if self.respond_to?(:after_update, true)
self
end
@@ -220,24 +220,24 @@ def update!(properties)
#
# @private
def _update!
+ repo = self.class.repository_or_fail
self.class.properties.each do |property, predicate|
+ value = attribute_get(property)
if dirty?(property)
- self.class.repository_or_fail.delete([subject, predicate[:predicate], nil])
+ repo.delete([subject, predicate[:predicate], nil])
if self.class.is_list?(property)
- repo = RDF::Repository.new
- attribute_get(property).each do |value|
- repo << RDF::Statement.new(subject, predicate[:predicate], self.class.build_rdf_value(value, self.class.properties[property][:type]))
+ value.each do |val|
+ store_attribute(property, val, predicate[:predicate], repo)
end
- self.class.repository_or_fail.insert(*repo)
else
- self.class.repository_or_fail.insert(RDF::Statement.new(subject, predicate[:predicate], self.class.build_rdf_value(attribute_get(property), self.class.properties[property][:type]))) unless attribute_get(property).nil?
+ store_attribute(property, value, predicate[:predicate], repo)
end
end
- @attributes[:original][property] = attribute_get(property)
+ @attributes[:original][property] = value
@dirty[property] = nil
@attributes[:copied][property] = NOT_SET
end
- self.class.repository_or_fail.insert(RDF::Statement.new(@subject, RDF.type, type)) unless type.nil?
+ repo.insert(RDF::Statement.new(@subject, RDF.type, type)) if type
end
##
@@ -361,21 +361,18 @@ def attribute_get(name)
# @param [Hash] attributes The attributes to create a repository for
# @private
def repository_for_attributes(attributes)
- repo = RDF::Repository.new
- attributes.each do | name, attribute |
- if self.class.is_list?(name)
- new = []
- attribute.each do |value|
- value = self.class.build_rdf_value(value, self.class.properties[name][:type])
- new << RDF::Statement.new(@subject, self.class.properties[name][:predicate], value)
+ RDF::Repository.new.tap do |repo|
+ attributes.each do | name, attribute |
+ predicate = self.class.properties[name][:predicate]
+ if self.class.is_list?(name)
+ attribute.each do |value|
+ store_attribute(name, value, predicate, repo)
+ end
+ else
+ store_attribute(name, attribute, predicate, repo)
end
- repo.insert(*new)
- else
- value = self.class.build_rdf_value(attribute, self.class.properties[name][:type])
- repo.insert(RDF::Statement.new(@subject, self.class.properties[name][:predicate], value))
end
end
- repo
end
##
@@ -562,6 +559,16 @@ def rename!(new_subject)
## Include the base validation functions
include Spira::Resource::Validations
+
+ private
+
+ def store_attribute(property, value, predicate, repository)
+ if value
+ val = self.class.build_rdf_value(value, self.class.properties[property][:type])
+ repository.insert(RDF::Statement.new(subject, predicate, val))
+ end
+ end
+
end
end
end
View
55 spec/hooks.spec
@@ -204,8 +204,61 @@ describe 'Spira resources' do
# This one makes sure that after_destory got called at all
@repository.should_not have_predicate RDF::FOAF.other
end
+ end
+ context "when the hook methods are private" do
+ before :all do
+ class ::PrivateHookTest < ::HookTest
+ type FOAF.bc_test
- end
+ def counter
+ @counter ||= {}
+ end
+ private
+
+ def before_create
+ self.counter[__method__] = true
+ end
+
+ def before_save
+ self.counter[__method__] = true
+ end
+
+ def before_destroy
+ self.counter[__method__] = true
+ end
+
+ def after_create
+ self.counter[__method__] = true
+ end
+
+ def after_save
+ self.counter[__method__] = true
+ end
+
+ def after_update
+ self.counter[__method__] = true
+ end
+
+ def after_destroy
+ self.counter[__method__] = true
+ end
+ end
+ end
+
+ before :each do
+ @repository << RDF::Statement.new(@subject, RDF.type, RDF::FOAF.bc_test)
+ end
+
+ it "should call the hook methods" do
+ subject = RDF::URI.new('http://example.org/test1').as(::PrivateHookTest)
+
+ subject.save!
+ subject.update!(:name => "Jay")
+ subject.destroy!
+
+ subject.counter.keys.count.should eql(7)
+ end
+ end
end
Something went wrong with that request. Please try again.