Navigation Menu

Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

Commit

Permalink
do_sqlite3 expects :database and :path options to be Strings
Browse files Browse the repository at this point in the history 
Once do_sqlite3 accepts both a Pathname or a String,
normalizing the database and path won't be necessary
anymore

[#1324 state_resolved]
  • Loading branch information
@postmodern @snusnu
postmodern authored and snusnu committed Jun 27, 2010
1 parent 2a8562b commit 528fe37
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/dm-sqlite-adapter/adapter.rb
View file
Expand Up @@ -17,7 +17,12 @@ def supports_subquery?(query, source_key, target_key, qualify)
end

def normalize_options(options)
options.update(:adapter => 'sqlite3')
# TODO Once do_sqlite3 accepts both a Pathname or a String,
# normalizing database and path won't be necessary anymore
db = (options[:database] || options['database']).to_s
path = (options[:path ] || options['path' ]).to_s

options.update(:adapter => 'sqlite3', :database => db, :path => path)
end

end
Expand Down
10 changes: 10 additions & 0 deletions spec/adapter_spec.rb
View file
Expand Up @@ -29,4 +29,14 @@
it { subject.options[:adapter].should == 'sqlite3' }
end

describe "with 'database' given as Symbol" do
subject { DataMapper::Adapters::SqliteAdapter.new(:default, { :adapter => 'sqlite', :database => :name }) }
it { subject.options[:database].should == 'name' }
end

describe "with 'path' given as Symbol" do
subject { DataMapper::Adapters::SqliteAdapter.new(:default, { :adapter => 'sqlite', :path => :name }) }
it { subject.options[:path].should == 'name' }
end

end

4 comments on commit 528fe37

@dkubb
Copy link
Member

@dkubb dkubb commented on 528fe37 Jun 27, 2010

Choose a reason for hiding this comment

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

If the 'database' key is a string, this code does not remove the key from the Hash. The options the Hash will contain { :database => db, 'database' => maybe_a_path } among other key/value pairs. When :database and 'database' are passed in as keys to do_sqlite3, I wonder which one will win?

Perhaps the code should become something like:

db   = (options[:database] || options.delete('database')).to_s
path = (options[:path    ] || options.delete('path')).to_s

@snusnu
Copy link
Member

@snusnu snusnu commented on 528fe37 Jun 27, 2010

Choose a reason for hiding this comment

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

Thx for catching this Dan! Fixed in http://github.com/datamapper/dm-sqlite-adapter/commit/3e90a58e933c07235a56a9d85b1dc94efe7844cb

@dkubb
Copy link
Member

@dkubb dkubb commented on 528fe37 Jun 27, 2010

Choose a reason for hiding this comment

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

@snusnu: great! another option might be to transform the incoming Hash into a Mash (HashWithIndifferentAccess in AS), which might be a better long-term approach because this sort of thing could creep up in the future.

I'm also kind of anal about this, but I tend to do things like check for the presence of the key before doing anything, so I might even consider something like: (assuming that options is a Mash)

%w[ database path ].each do |key|
  options.update(key => options.fetch(key).to_s) if options.key?(key)
end

@postmodern
Copy link
Member Author

Choose a reason for hiding this comment

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

I am all for converting the Hash given to DataMapper into a Mash. When I first wrote the patch I was unsure whether DataMapper was normalizing the Hash, or if it expected the calling-library or Rails to handle that.

Please sign in to comment.