ruby-rdf / rdf-ldp

A suite of LDP software and middleware for RDF.rb & Rack
The Unlicense
13 stars 2 forks source link

Explicitly set StorageAdapter #63

Closed mjsuhonos closed 8 years ago

mjsuhonos commented 8 years ago

Add a method #set_storage that allows setting the StorageAdapter for a NonRDFSource if it has not already been set.

mjsuhonos commented 8 years ago

Sorry, this PR unintentionally includes #62 (there is no dependency); closing for now.

no-reply commented 8 years ago

I merged #62 (the spec is #64). Do you want to rebase this?


What do you think about injecting the adapter dependency at initialization time? I'm thinking we should back off the mutable state @storage_adapter ||= approach, removing the need for the return false if @storage_adapter check.

Something like this:

DEFAULT_ADAPTER = RDF::LDP::NonRDFSource::StorageAdapter

attr_reader :storage

def initialize(..., storage_adapter: DEFAULT_ADAPTER)
  super
  @storage = storage_adapter.new
end

With either option, I'm not sure what is required at https://github.com/ruby-rdf/rdf-ldp/blob/develop/lib/rdf/ldp/resource.rb#L131-L135 to make this cleanly configurable. Do you have something in mind, there?

mjsuhonos commented 8 years ago

:+1: I prefer dependency injection over mutation in general; the current approach was on to minimize changes to the code. I'm not crazy about using a constant for the default StorageAdapter type, but I'll take a stab at something in the next while.

no-reply commented 8 years ago

I'm not crazy about using a constant for the default StorageAdapter type

The advantage of a constant, over a hard coded default is that it's easy to override in a subclass. You get DEFAULT_ADAPTER = MyAdapter instead of def initialize(..., storage_adapter = MyAdapter.new); super; end (and cross your finger nothing else is also hard coded to the base class's default adapter class.

mjsuhonos commented 8 years ago

Right, point fairly made. This sounds like it would be a good solution to https://github.com/ActiveTriples/ActiveTriples/issues/198 as well.

mjsuhonos commented 8 years ago

This commit introduces some failures related to using named parameters for storage_adapter. Help welcome.

no-reply commented 8 years ago

Travis build doesn't seem to be triggering. Can you say more about the errors you're seeing?

mjsuhonos commented 8 years ago
Failures:

  1) RDF::LDP::NonRDFSource behaves like a NonRDFSource #create persists to resource
     Failure/Error: raise NotFound if graph.empty?

     RDF::LDP::NotFound:
       RDF::LDP::NotFound
     Shared Example Group: "a NonRDFSource" called from ./spec/lib/rdf/ldp/non_rdf_source_spec.rb:4
     # ./lib/rdf/ldp/resource.rb:129:in `find'
     # ./spec/support/shared_examples/non_rdf_source.rb:34:in `block (3 levels) in <top (required)>'

  2) RDF::LDP::NonRDFSource behaves like a NonRDFSource #create creates an LDP::RDFSource
     Failure/Error:
       expect { saved.create(contents, 'text/plain') }
         .to change { description.exists? }.from(false).to(true)

       expected result to have changed from false to true, but did not change
     Shared Example Group: "a NonRDFSource" called from ./spec/lib/rdf/ldp/non_rdf_source_spec.rb:4
     # ./spec/support/shared_examples/non_rdf_source.rb:43:in `block (3 levels) in <top (required)>'