Skip to content
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

Open
akxcv opened this issue Mar 3, 2017 · 8 comments · May be fixed by #242
Open

Sequel support #28

akxcv opened this issue Mar 3, 2017 · 8 comments · May be fixed by #242

Comments

@akxcv
Copy link
Contributor

akxcv commented Mar 3, 2017

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
Copy link
Owner

palkan commented Mar 3, 2017

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
Copy link
Contributor Author

akxcv commented Mar 3, 2017

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
Copy link
Owner

palkan commented Mar 3, 2017

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
Copy link
Contributor Author

akxcv commented Mar 3, 2017

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
Copy link
Owner

palkan commented Mar 3, 2017

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

@akxcv
Copy link
Contributor Author

akxcv commented Mar 3, 2017

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

@akxcv
Copy link
Contributor Author

akxcv commented Mar 3, 2017

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
Copy link
Contributor Author

akxcv commented Mar 13, 2017

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.

@snacks02 snacks02 linked a pull request Oct 26, 2023 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants