Conversation
scarroll32
force-pushed
the
refactor-adapters
branch
from
a93e94d to
1759232
Compare
|
@deivid-rodriguez could you please review this refactoring. It should go after #1345 I had intended to completely remove the |
|
@deivid-rodriguez can we get this into the 4.0.0 release also? |
|
I can try to review it, can you fix conflicts? |
@deivid-rodriguez sorry I didn't realise it had conflicts, will fix asap. |
scarroll32
force-pushed
the
refactor-adapters
branch
from
cc6b2aa to
942a948
Compare
|
It's ready for review now @deivid-rodriguez |
deivid-rodriguez
left a comment
deivid-rodriguez
left a comment
There was a problem hiding this comment.
In general the refactoring looks good. We lose the flexibility of adding more adapters in the future, and people lose the flexibility to do it themselves too by implementing the necessary classes. But I guess we have enough with ActiveRecord?
lib/ransack.rb
Outdated
| require 'ransack/adapters/active_record' | ||
| require 'ransack/adapters/active_record/ransack/visitor' | ||
| require 'ransack/adapters/active_record/ransack/nodes/condition' | ||
| require 'ransack/adapters/active_record/ransack/context' |
There was a problem hiding this comment.
If I'm following this refactoring correctly, only the first two requires above are needed?
There was a problem hiding this comment.
I'll check this and get back to you @deivid-rodriguez
There was a problem hiding this comment.
But I guess we have enough with ActiveRecord?
@deivid-rodriguez I think we keep it to ActiveRecord only, we already removed MongoDB.
lib/polyamorous.rb
Outdated
| @@ -1 +0,0 @@ | |||
| require 'polyamorous/polyamorous' | |||
There was a problem hiding this comment.
I think this was some compatibility code for people using the old polyamorous gem when it lived separately. We could remove it too, but seems unrelated to refactoring the adapters, right?
There was a problem hiding this comment.
@deivid-rodriguez I found this line to be unnecessary, but you are right: it is unrelated to the adapter refactoring. I will pull it into it's own PR.
| i18n_alias :a => :attribute, :p => :predicate, | ||
| :m => :combinator, :v => :value | ||
| i18n_alias a: :attribute, p: :predicate, | ||
| m: :combinator, v: :value |
There was a problem hiding this comment.
I like this style improvement too (or the others similar to this), but maybe we can enable it separately and enforce it through RuboCop?
There was a problem hiding this comment.
Good point @deivid-rodriguez, I'll pull this into it's own PR.
|
@deivid-rodriguez I will look at these comments over the weekend. I wonder if we should hold the 4.0.0 release so that this can come in? It is not a breaking change, but it is a large refactoring. I can go either way (either 4.0.0 or 4.1.0) WDYT ? 🤔 |
|
I think we can hold it until you can have a look. Breaking Thanks for this @scarroll32! |
scarroll32
force-pushed
the
refactor-adapters
branch
from
942a948 to
4081b12
Compare
scarroll32
force-pushed
the
refactor-adapters
branch
from
4081b12 to
880629d
Compare
scarroll32
force-pushed
the
refactor-adapters
branch
from
880629d to
d0241da
Compare
scarroll32
force-pushed
the
refactor-adapters
branch
from
d0241da to
70e8238
Compare
|
@deivid-rodriguez could you please take another look at this in light of #1371 and #1370 ? |
|
@deivid-rodriguez are you OK to merge this? |
deivid-rodriguez
left a comment
deivid-rodriguez
left a comment
There was a problem hiding this comment.
@scarroll32 Yeah, sure. I haven't suffered the pain from maintaining these adapters, but if this is painful and no other ORM framework is supported other than ActiveRecord anyways, then it makes sense to get rid of all this.
|
This was a pretty bad decision. Even if someone would want to maintain the mongoid adapter, or create another one, now it is impossible. |
3 participants
Closes #1346