-
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
Restructure querying into plugin #216
Conversation
p.s. currently I'm focusing exclusively on ActiveRecord, but if/once I get this working I'll try to do a similar refactor for Sequel. |
76c905f
to
6220ea3
Compare
Specs are passing locally for Table, Jsonb, Json and Hstore backends. Container is just a bit more work, KeyValue will be slightly trickier but generally this seems to work well. |
5f106a2
to
b2a9ab8
Compare
Ok this is working now locally for all AR backends. Still need to clean up a lot and support Sequel, but this chops off 300+ lines of code. ✂️ |
e7640cf
to
4d9528d
Compare
やった! This is finally cleaned up and passing specs + code climate (after a few tweaks). Chopped 250 lines and I think the code is cleaner but also more powerful. I'll need to add some docs and squash, etc., I figure the final line drop will be more in the ballpark of minus 200, which is still pretty good. |
Rebased on top of #218 |
d3ca0db
to
0558cb5
Compare
I've left this as one giant commit, because to split it is more work that it is worth. This change replaces module builders for each backend with a single plugin which delegates to the methods build_node (which returns an Arel node) and (optionally) add_translations (which modifies an AR::Relation) on the backend class. Backend classes now need only to implement these one or two methods and querying is supported without any extra work.
Rebased on #219. |
This is far from complete, but I wanted to create a WIP right away to explain it since it's a big change with some benefits for anyone using Mobility with querying.
The change is to migrate the querying support offered by Mobility into a shared plugin. Rather than each backend independently overriding methods like
where
, the plugin does this overriding in one place and calls two methods on the backend class, which each backend can customize.These methods are
build_node
andadd_scope
(exact names may change). Each backend only needs to implement these methods to support querying.build_node
takes an attribute name, a locale (currentlyMobility.locale
) and a set of backend options and returns an Arel node.add_scope
takes a relation, a hash of attributes and values, and a hash of backend options and returns an (optionally) modified relation.Why is this important? Because it means that we can fully abstract how backends handle querying into these two methods. e.g. for the Table backend (for which I've got specs now passing),
build_node
creates a node for the column on the translation table, whereasadd_scope
joins the translation table to the model table on the relation.For the jsonb backend, the node would be e.g.
post.title -> 'en'
, for an English locale, etc.This is really powerful because it means that we can now do more complex querying. e.g. suppose you want to case-sensitively search for a translated value independent of backend. With this change, if you get the node with
build_node
, you can then build a predicate withnode.lower.eq("foo")
. Previously, there was no backend-independent way of doing this.This means that e.g. the uniqueness validator could support case-insensitivity, fixing #141. But it also means it would be possible to build extension gems to support integration with e.g. Ransack, etc. in a backend-independent way.
The code here is quite complex but when this is all done, things should be simpler since all the backend-specific
QueryMethods
classes will be removed, so complexity will be captured exclusively by one (plugin) class.