Refactor adapters

[scarroll32]

Closes #1346

[scarroll32]

@deivid-rodriguez could you please review this refactoring. It should go after #1345

I had intended to completely remove the Ransack::Adapter::ActiveRecord namespace and replace it with Ransack::Adapter::ActiveRecord but I could not get past a connection error. It can be a follow-up issue.

[scarroll32]

@deivid-rodriguez can we get this into the 4.0.0 release also?

[deivid-rodriguez]

I can try to review it, can you fix conflicts?

[scarroll32]

I can try to review it, can you fix conflicts?

@deivid-rodriguez sorry I didn't realise it had conflicts, will fix asap.

[scarroll32]

It's ready for review now @deivid-rodriguez

[deivid-rodriguez]

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?

[deivid-rodriguez]

If I'm following this refactoring correctly, only the first two requires above are needed?

[scarroll32]

I'll check this and get back to you @deivid-rodriguez

[scarroll32]

But I guess we have enough with ActiveRecord?

@deivid-rodriguez I think we keep it to ActiveRecord only, we already removed MongoDB.

[deivid-rodriguez]

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?

[scarroll32]

@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.

[deivid-rodriguez]

I like this style improvement too (or the others similar to this), but maybe we can enable it separately and enforce it through RuboCop?

[scarroll32]

Good point @deivid-rodriguez, I'll pull this into it's own PR.

[scarroll32]

@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 ? 🤔

[deivid-rodriguez]

I think we can hold it until you can have a look. Breaking require "polyamorous" could be considered a breaking change so I'd ship it with 4.0.0 too.

Thanks for this @scarroll32!

[scarroll32]

@deivid-rodriguez could you please take another look at this in light of #1371 and #1370 ?

[scarroll32]

@deivid-rodriguez are you OK to merge this?

[deivid-rodriguez]

@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.