logidze icon indicating copy to clipboard operation
logidze copied to clipboard

Sequel support

Open akxcv opened this issue 7 years ago • 8 comments

Hi! It would be awesome if this gem supported not only ActiveRecord, but Sequel, too. I fiddled around with it the other day and it seems that providing Sequel support isn't hard. What do you think about it? Should we make an adapter that would detect what abstraction is being used or should we make a separate gem for Sequel?

akxcv avatar Mar 03 '17 05:03 akxcv

I'd prefer to start with Sequel support within this gem. Otherwise we have to extract core functionality into, say, logidze-core and thus support at least 3 gems: logidze (for ActiveRecord), logidze-core and logidze-sequel. And most of the features would require changes in both core and specific adapter projects.

Can you describe, please, how do you expect Logidze to work with Sequel?

palkan avatar Mar 03 '17 10:03 palkan

We could start by using sequel-rails. It provides a migration API that is almost identical to ActiveRecord's one. For database queries and models we could probably use adapters.

For example:

Logidze::DB.transaction do
  Logidze::DB.execute(...)
end

The DB abstraction would pick the right adapter to use automatically.

I think, much of the difficulty will be in testing. It's not hard to rewrite tests for Sequel, I just don't quite know how to organize them.

akxcv avatar Mar 03 '17 11:03 akxcv

First of all, we should extract SQL generation from migrations to make it possible to use them in both AR and Sequel migrations without any modification. Smth like:

class LogdizeAR < ActiveRecord::Migration
  def up
    execute Logidze::SQ::Install.up(upgrade: true)
  end

  def down
    execute Logidze::SQL::Install.down
  end
end

Sequel.migration do
  up { run Logidze::SQL::Install.up(upgrade: true) }
  down { run Logidze::SQL::Install.down }
end

We can event do that the Rails way using custom commands:

class LogdizeInstall < ActiveRecord::Migration
  def change
    create_logidze
  end
end

class LogdizePost < ActiveRecord::Migration
  def change
    add_logidze :posts, timestamp_column: :published_at, whitelist: %w(title body)
  end
end

palkan avatar Mar 03 '17 12:03 palkan

Actually, we don't have to extract any SQL. I was able to create a Sequel migration just by changing:

class <%= @migration_class_name %> < ActiveRecord::Migration
  require 'logidze/migration'
  include Logidze::Migration

  def up
    # stuff
  end
  
  # ...
end

To:

require 'logidze/migration'
include Logidze::Migration

Sequel.migration do
  up do
    # exactly the same stuff
  end
  # ...
end

So maybe we should extract everything but SQL generation since it's the same anyways?

akxcv avatar Mar 03 '17 13:03 akxcv

Sorry, I don't understand what do you mean by "extract everything but SQL generation"

palkan avatar Mar 03 '17 13:03 palkan

On a second thought, my idea is pretty weird. :) You are right, we should extract SQL generation into separate erb templates.

akxcv avatar Mar 03 '17 13:03 akxcv

I guess we should do the same for all ActiveRecord invocations.

We'll need a database wrapper to encapsulate ActiveRecord::Base.connection and Sequel::Model.db (Logidze::DB?). Also, we'll need a model wrapper to encapsulate ActiveRecord::Base and Sequel::Model (Logidze::Model if we rename the existing module to Logidze::ModelExtensions?)

How should we detect which one to use, AR or Sequel?

akxcv avatar Mar 03 '17 14:03 akxcv

An update on this: sequel-rails doesn't seem to be actively maintained, and, most likely, isn't used much. Looks like we'll have to go without it.

akxcv avatar Mar 13 '17 13:03 akxcv