-
Notifications
You must be signed in to change notification settings - Fork 87
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
Make ActiveRecord and Sequel actual plugins #414
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shioyama
force-pushed
the
activerecord_plugin
branch
2 times, most recently
from
July 10, 2020 05:35
c6daf66
to
7d07303
Compare
shioyama
force-pushed
the
activerecord_plugin
branch
from
July 26, 2020 05:37
7d07303
to
e6aa7db
Compare
shioyama
commented
Jul 26, 2020
raise unless e.message =~ /sequel/ | ||
Loaded::Sequel = false | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be so happy when this is all gone.
shioyama
force-pushed
the
activerecord_plugin
branch
17 times, most recently
from
August 2, 2020 03:34
11bc97e
to
efabb23
Compare
shioyama
force-pushed
the
activerecord_plugin
branch
4 times, most recently
from
August 6, 2020 02:28
8a9aa5a
to
69a9bdd
Compare
Previously, plugins like Dirty would branch out to different implementations from the includes hook depending on the including class, since ActiveRecord and Sequel must be handled differently. This meant that code that should be generic has to know about every possible ORM-specific variant of itself. Here we use Inversion of Control by making the reasonable assumption that an Attributes module is configured for a particular ORM model. We can then flip this situation around so that active_record and sequel are real plugins which themselves call ORM-specific implementations of generic plugins. So, for example, if you have: ``` class TranslatedAttributes < Mobility::Attributes plugin :active_record plugin :dirty end ``` Then the active_record plugin includes active_record_dirty, which itself depends on dirty. This way, the AR specific dirty code is only triggered if you actually enable dirty. Same for Sequel. With this change, basically everything under lib/mobility/plugins is actually a plugin, managing its own dependencies.
shioyama
force-pushed
the
activerecord_plugin
branch
from
August 6, 2020 02:37
69a9bdd
to
4a84940
Compare
shioyama
force-pushed
the
activerecord_plugin
branch
from
August 6, 2020 03:04
a90e71a
to
9a8d114
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently on
master
,Mobility::Plugins::ActiveRecord
,Mobility::Plugins::ActiveModel
andMobility::Plugins::Sequel
are simply namespaces which hold modules called byMobility::Plugins::Dirty
depending on what gems are loaded.This is really ugly, because it means that code that should be generic has to know every possible ORM variant.
With this PR, we use inversion of control to flip this situation around so that
active_record
andsequel
are real plugins which themselves callactive_record_dirty
orsequel_dirty
,active_record_cache
orsequel_cache
, etc. These latter plugins depend are triggered by inclusion of the generic ones (dirty
,cache
).So basically, if you have:
then the
active_record
plugin includesactive_record_dirty
, which itself depends ondirty
(withinclude: false
). This way, the AR specific dirty code is only triggered if you actually enabledirty
. Same for Sequel.The other thing that happens here is that we assume that if you have
plugin :active_record
, the model you're translating must be anActiveRecord::Base
model. Previously, the Dirty plugin would trigger on theincluded
hook and check which class it was included into, but this meant that the model class would get more modules than necessary.Now, everything is explicit and you must declare
plugin :active_record
, and when you do, Mobility expects translated classes to be ActiveRecord classes, etc. If (in that 0.001% of cases) you are using Mobility with both ActiveRecord and Sequel, you'd just have to create two module builders, one for each:So since everything is now more modular, this is still entirely possible.
The nice thing here is that all the ugly code which was implicitly trying to figure out what gems are loaded (to automatically switch between ORM classes) is gone. I think explicitness here is much clearer and safer.
Same goes for backends: the
active_record
plugin loads anactive_record_backend
plugin which overridesload_backend
to first search for a backend underMobility::Backends::ActiveRecord
(same forsequel_backend
). This again means a lot of magic becomes less magical, which I think is good.Update: I've also extracted UniquenessValidation into its own plugin, which is automatically included when you call
plugin :active_record
.