Add Sequel preliminary support #229
Conversation
| extend Logidze::Meta | ||
| require "logidze/adapter/base" | ||
| require "logidze/adapter/active_record" | ||
| require "logidze/adapter/sequel" |
There was a problem hiding this comment.
Shouldn't we check for defined?(::Sequel) here?
There was a problem hiding this comment.
It's safe to reference Sequel there as within the file we call Sequel inside method calls only.
However, it's always better to reduce allocated classes so indeed we should conditionally load.
| require "logidze/meta" | ||
|
|
||
| extend Logidze::Meta | ||
| require "logidze/adapter/base" |
There was a problem hiding this comment.
The "adapter" name is too generic; what about "database_adapter"? Or "connection_adapters" like in Active Record.
There was a problem hiding this comment.
Let's stick with the Rails naming.
| @@ -190,6 +194,28 @@ bundle exec rails destroy logidze:model Post | |||
|
|
|||
| **IMPORTANT**: If you use non-UTC time zone for Active Record (`config.active_record.default_timezone`), you MUST always infer log timestamps from a timestamp column (e.g., when back-filling data); otherwise, you may end up with inconsistent logs ([#199](https://github.com/palkan/logidze/issues/199)). In general, we recommend using UTC as the database time unless there is a very strong reason not to. | |||
|
|
|||
| ### Sequel support | |||
|
|
|||
| All mentioned generators also optionally support Sequel. | |||
There was a problem hiding this comment.
We should mention here that the current Sequel support is more like "Sequel within Rails" support, since we still re-use a lot of Rails features. Splitting the library to work with pure Sequel (and without any Rails stuff) will be a next step (some day in the future).
There was a problem hiding this comment.
I'll mention the limitations, thanks.
By the way, Sequel support does not need Rails extensively - we use it for JSON encode / decode only.
Of course, Sequel offers the same facilities to solve this problem too.
Maybe, fully separate modes are nearer than it looks ;)
|
Closed in favor of #242 |
Hi, Vladimir!
What is the purpose of this pull request?
Add Sequel preliminary support.
It's an important feature for the Sequel ecosystem as, AFAIK, there's no model change trackers at all.
Fixes #28
What changes did you make? (overview)
It supports:
Logidze.with_responsible,Logidze.without_logging)Preliminary support means that although Active Record is decoupled already, it's still a hard dependency for the gemspec in order to minimize any possible breaks.
However, there should be no breaking changes for the current users.
Moreover, it removes:
It's hidden now in implementation adapters, only public API is included to models.
ignores_log_dataglobal model pollutionThe method was included to all ActiveRecord::Base models; it's included only with
has_logidzenow.It's somewhat breaking change but it seems unlikely that someone uses a Logidze method on models without Logidze included.
log_versionpossible breakage with nulllog_data(for example, no version at some range)It returns
nilnow.Is there anything you'd like reviewers to focus on?
Fun fact: a really crude version of this code is saved one real world project with Sequel in 2020
(manually applied triggers and Logidze::History).
You know, an application-independent tracker rules! Thank you!
Checklist