Skip to content
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

Refactor adapters #1348

Merged
merged 8 commits into from
Nov 22, 2022
Merged

Refactor adapters #1348

merged 8 commits into from
Nov 22, 2022

Conversation

scarroll32
Copy link
Member

@scarroll32 scarroll32 commented Jun 12, 2022

Closes #1346

@scarroll32 scarroll32 marked this pull request as draft June 12, 2022 15:09
@scarroll32 scarroll32 marked this pull request as ready for review June 12, 2022 16:34
@scarroll32
Copy link
Member Author

scarroll32 commented Jun 12, 2022

@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
Copy link
Member Author

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

@deivid-rodriguez
Copy link
Contributor

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

@scarroll32
Copy link
Member Author

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

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

@scarroll32
Copy link
Member Author

It's ready for review now @deivid-rodriguez

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I guess we have enough with ActiveRecord?

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

@@ -1 +0,0 @@
require 'polyamorous/polyamorous'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@scarroll32
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

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

@scarroll32
Copy link
Member Author

@deivid-rodriguez are you OK to merge this?

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@scarroll32 scarroll32 merged commit 0070757 into main Nov 22, 2022
@scarroll32 scarroll32 deleted the refactor-adapters branch November 22, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor adapter code
2 participants