Adding support for DataMapper #24

Merged
merged 1 commit into from Jan 30, 2013

Conversation

Projects
None yet
3 participants
@filipesabella
Contributor

filipesabella commented Jan 28, 2013

Basically we had to change every usage of

ActiveRecord::Base.connection

to use the new abstraction we created.

This led to more problems than expected when dealing with threading issues and connection availability to each thread.

lib/lhm/connection.rb
+ DataMapperConnection.new(adapter)
+ elsif defined?(ActiveRecord)
+ ActiveRecordConnection.new(adapter)
+ end

This comment has been minimized.

Show comment Hide comment
@grobie

grobie Jan 28, 2013

Member

What about raising an exception here, if neither DataMapper nor ActiveRecord are available? Otherwise Lhm will probably error later in a weird way.

@grobie

grobie Jan 28, 2013

Member

What about raising an exception here, if neither DataMapper nor ActiveRecord are available? Otherwise Lhm will probably error later in a weird way.

Rakefile
@@ -11,10 +11,9 @@ end
Rake::TestTask.new("integration") do |t|
t.libs.push "lib"
- t.test_files = FileList['spec/integration/*_spec.rb']
+ t.test_files = FileList[ENV['SPEC'] || 'spec/integration/*_spec.rb']

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Jan 30, 2013

Contributor

Rake::TestTask already does this for you, but the environment variable is named TEST: https://github.com/jimweirich/rake/blob/master/lib/rake/testtask.rb#L136

@lgierth

lgierth Jan 30, 2013

Contributor

Rake::TestTask already does this for you, but the environment variable is named TEST: https://github.com/jimweirich/rake/blob/master/lib/rake/testtask.rb#L136

lib/lhm.rb
+ raise 'Please call Lhm.setup before Lhm.change_table'
+ end
+
+ @@adapter

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Jan 30, 2013

Contributor

You can shorten this using ||=. var ||= 123 is basically the same as var = var || 123.

def self.adapter
  @@adapter ||=
    begin
      raise 'Please call Lhm.setup' unless defined?(ActiveRecord)
      ActiveRecord::Base.connection
    end
end
@lgierth

lgierth Jan 30, 2013

Contributor

You can shorten this using ||=. var ||= 123 is basically the same as var = var || 123.

def self.adapter
  @@adapter ||=
    begin
      raise 'Please call Lhm.setup' unless defined?(ActiveRecord)
      ActiveRecord::Base.connection
    end
end
lib/lhm/connection.rb
+ elsif defined?(ActiveRecord)
+ ActiveRecordConnection.new(adapter)
+ else
+ raise 'Nor DataMapper or ActiveRecord found.'

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Jan 30, 2013

Contributor

Neither DataMapper nor ActiveRecord

@lgierth

lgierth Jan 30, 2013

Contributor

Neither DataMapper nor ActiveRecord

lib/lhm/sql_helper.rb
+ classes = []
+ classes << DataObjects::SQLError if defined?(DataMapper)
+ classes << ActiveRecord::StatementInvalid if defined?(ActiveRecord)
+ classes

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Jan 30, 2013

Contributor

It looks like error_classes is an artifact from me.

@lgierth

lgierth Jan 30, 2013

Contributor

It looks like error_classes is an artifact from me.

@lgierth

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Jan 30, 2013

Contributor

Very very cool stuff! 😻

Contributor

lgierth commented Jan 30, 2013

Very very cool stuff! 😻

@lgierth

This comment has been minimized.

Show comment Hide comment
@lgierth

lgierth Jan 30, 2013

Contributor

🚀

Let's test this with that internal project and then get 1.2.0 out. /cc @grobie

Contributor

lgierth commented Jan 30, 2013

🚀

Let's test this with that internal project and then get 1.2.0 out. /cc @grobie

@grobie

This comment has been minimized.

Show comment Hide comment
@grobie

grobie Jan 30, 2013

Member

👍 go for it :)

On Wed, Jan 30, 2013 at 3:50 PM, Lars Gierth notifications@github.comwrote:

[image: 🚀]

Let's test this with that internal project and then get 1.2.0 out. /cc
@grobie https://github.com/grobie


Reply to this email directly or view it on GitHubhttps://github.com/soundcloud/large-hadron-migrator/pull/24#issuecomment-12892707.

Member

grobie commented Jan 30, 2013

👍 go for it :)

On Wed, Jan 30, 2013 at 3:50 PM, Lars Gierth notifications@github.comwrote:

[image: 🚀]

Let's test this with that internal project and then get 1.2.0 out. /cc
@grobie https://github.com/grobie


Reply to this email directly or view it on GitHubhttps://github.com/soundcloud/large-hadron-migrator/pull/24#issuecomment-12892707.

Adding support for DataMapper
Basically we had to change every usage of

ActiveRecord::Base.connection
to use the new abstraction we created.

This led to more problems than expected when dealing with threading issues and connection availability to each thread.

filipesabella added a commit that referenced this pull request Jan 30, 2013

@filipesabella filipesabella merged commit dcf2335 into master Jan 30, 2013

1 check passed

default The Travis build passed
Details

@filipesabella filipesabella deleted the datamapper branch Jan 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment