-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sequel support #28
Comments
I'd prefer to start with Sequel support within this gem. Can you describe, please, how do you expect Logidze to work with Sequel? |
We could start by using 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. |
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 |
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? |
Sorry, I don't understand what do you mean by "extract everything but SQL generation" |
On a second thought, my idea is pretty weird. :) |
I guess we should do the same for all We'll need a database wrapper to encapsulate How should we detect which one to use, AR or Sequel? |
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. |
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?
The text was updated successfully, but these errors were encountered: