Remove unused Arel visitors in the code base #38946
Conversation
|
I understand and welcome this change, but it would be really nice to actually get in sync with adapter authors and ensure they could prepare new release including visitor before this is released as well on Rails side. Also temporarily it would be developer friendly to raise error on request to use one of those removed visitors. |
|
It is good to alert the adapters authors but I don't think we should be waiting them to respond. Even more that we don't know all of them. This is a project decision, like any other API we provide. Specially arel as it is private API. I don't think an error is necessary if all actually maintained adapters change their code, which will be required anyway. We already alerted the oracle adapter maintainers, now we need to alert the mssql adapter https://github.com/rails-sqlserver/activerecord-sqlserver-adapter |
|
Note that sqlserver adapter is not using Arel bundled mssql visiter but using its own visitor. |
|
I am fine this change, actually, I need some time to migrate Arel code into Oracle enhanced adapter. It may take couple of weeks. |
|
Anyone any idea about ibm_db and informix usage? |
kamipo
force-pushed
the
remove_unused_visitors
branch
from
7f847c7
to
19574e3
Compare
|
I have opened issues at both informix and ibm_db related repos I found. |
This removes ibm_db, informix, mssql, oracle, and oracle12 Arel visitors which are not used in the code base. Actually oracle and oracle12 visitors are used at oracle-enhanced adapter, but now I think that those visitors should be in the adapter's repo like sqlserver adapter and the dedicated Arel visitor (https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/arel/visitors/sqlserver.rb), otherwise it is hard to find a bug and review PRs for the oracle visitors (e.g. rails#35838, rails#37646), since we don't have knowledge and environment enough for Oracle.
kamipo
force-pushed
the
remove_unused_visitors
branch
from
19574e3
to
a2040ee
Compare
|
FYI Oracle enhanced adapter supports this change rsim/oracle-enhanced#2002 |
`in_clause_length` was added at c5a284f to address to Oracle IN clause length limitation. Now `in_clause_length` is entirely integrated in Arel visitor since rails#35838 and rails#36074. Since Oracle visitors are the only code that rely on `in_clause_length`. so I'd like to remove that from Rails code base, like has removed Oracle visitors (rails#38946).
This removes ibm_db, informix, mssql, oracle, and oracle12 Arel visitors
which are not used in the code base.
Actually oracle and oracle12 visitors are used at oracle-enhanced
adapter, but now I think that those visitors should be in the adapter's
repo like sqlserver adapter and the dedicated Arel visitor
(https://github.com/rails-sqlserver/activerecord-sqlserver-adapter/blob/master/lib/arel/visitors/sqlserver.rb),
otherwise it is hard to find a bug and review PRs for the oracle
visitors (e.g. #35838, #37646), since we don't have knowledge and
environment enough for Oracle.
cc @yahonda