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
Extract arel_visitor
and move up to the abstract adapter
#23515
Conversation
(@rails-bot has picked a reviewer for you, use r? to override) |
c48cd01
to
3cdc180
Compare
@@ -143,6 +149,10 @@ def collector | |||
end | |||
end | |||
|
|||
def arel_visitor # :nodoc: | |||
(Arel::Visitors::VISITORS[@config[:adapter]] || Arel::Visitors::ToSql).new(self) |
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.
Why fallback to Arel::Visitors::ToSql
?
I think it is better to override this method if someone create an adapter that Arel does not support.
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.
This code is derived from https://github.com/rails/arel/blob/v7.0.0/lib/arel/visitors.rb#L33.
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 understand, thanks !
3cdc180
to
4087c5c
Compare
4087c5c
to
c3508bf
Compare
c3508bf
to
8200b3b
Compare
Extract `arel_visitor` and move up to the abstract adapter
Is this a permanent change? It breaks Ransack's Active Record adapter: |
hmm... |
Fixed by #24634. |
…sage This attr writer was introduced at 7db90aa, but the usage is already removed at bd2f5c0 before v3.2.0.rc1 is released. If we'd like to customize the visitor in the connection, `arel_visitor` which is implemented in all adapters (mysql2, postgresql, sqlite3, oracle-enhanced, sqlserver) could be used for the purpose #23515.
No description provided.