palkan / logidze

Database changes log for Rails
MIT License
1.59k stars 75 forks source link

Sequel support #28

Open akxcv opened 7 years ago

akxcv commented 7 years ago

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?

palkan commented 7 years ago

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?

akxcv commented 7 years ago

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.

palkan commented 7 years ago

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
akxcv commented 7 years ago

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?

palkan commented 7 years ago

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

akxcv commented 7 years ago

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

akxcv commented 7 years ago

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 commented 7 years ago

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.