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 `SchemaDumper` to make it possible to adapter specific customization #30337

Merged
merged 2 commits into from Aug 24, 2017

Conversation

Projects
None yet
2 participants
@kamipo
Member

kamipo commented Aug 20, 2017

Currently SchemaDumper is only customizable for column options. But
3rd party connection adapters (oracle-enhanced etc) need to customizable
for table or index dumping also. To make it possible, I introduced
adapter specific SchemaDumper classes for that.

cc @yahonda

@yahonda

This comment has been minimized.

Contributor

yahonda commented Aug 21, 2017

Thanks for the pull request and update. I'm testing this pull request with Oracle enhanced adapter. So far I need to address "undefined method column_spec_for_primary_key"
https://gist.github.com/yahonda/18d4ae7317701188843a1d61e082a5c4

Let me have some more time to take a look at this pull request. I think I will have some questions.

@kamipo

This comment has been minimized.

Member

kamipo commented Aug 21, 2017

undefined method `column_spec_for_primary_key` is due to copying old table definition.

https://github.com/rsim/oracle-enhanced/blob/c463383d34cc113b0a394019cf89b0db076f2e52/lib/active_record/connection_adapters/oracle_enhanced/schema_dumper.rb#L124

All utility methods (column_spec and column_spec_for_primary_key as a public, and many internal private methods) that is used for schema dumping are moved in SchemaDumper and ColumnDumper is no longer included in the connection adapter.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Aug 21, 2017

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Aug 21, 2017

@yahonda

This comment has been minimized.

Contributor

yahonda commented Aug 21, 2017

Thanks for pointing out. I overlooked that part. Now I have one failure.
The remaining part is how to consolidate these two namespaces. #30337 renames ColumnDumper to SchemaDumper.
Then it will conflict.

  • ActiveRecord::ConnectionAdapters::OracleEnhanced::SchemaDumper
  • ActiveRecord::ConnectionAdapters::OracleEnhanced::ColumnDumper

Will keep working on it.

kamipo added a commit to kamipo/oracle-enhanced that referenced this pull request Aug 22, 2017

kamipo added a commit to kamipo/oracle-enhanced that referenced this pull request Aug 22, 2017

kamipo added some commits Aug 20, 2017

Refactor `SchemaDumper` to make it possible to adapter specific custo…
…mization

Currently `SchemaDumper` is only customizable for column options. But
3rd party connection adapters (oracle-enhanced etc) need to customizable
for table or index dumping also. To make it possible, I introduced
adapter specific `SchemaDumper` classes for that.
@yahonda

This comment has been minimized.

Contributor

yahonda commented Aug 22, 2017

rsim/oracle-enhanced#1430 resolves all errors, failures and my questions. I think it looks good to me.

@yahonda

This comment has been minimized.

Contributor

yahonda commented Aug 22, 2017

cc @metaskills who maintains ActiveRecord SQL Server Adapter.

kamipo added a commit to kamipo/activerecord-sqlserver-adapter that referenced this pull request Aug 22, 2017

@kamipo

This comment has been minimized.

Member

kamipo commented Aug 22, 2017

SQL Server Adapter is only customized for column options, so it is easy to follow to this change.
I've created rails-sqlserver/activerecord-sqlserver-adapter#617.

yahonda added a commit to rsim/oracle-enhanced that referenced this pull request Aug 23, 2017

yahonda added a commit to rsim/oracle-enhanced that referenced this pull request Aug 23, 2017

@kamipo kamipo merged commit f10a646 into rails:master Aug 24, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:refactor_schema_dumper branch Aug 24, 2017

yahonda added a commit to yahonda/rails that referenced this pull request Oct 25, 2017

Implement `PostgreSQL::SchemaDumper#extensions`
and abstract `SchemaDumper#extensions` is now an empty method.

Since rails#30337, every database adapter has its own `SchemaDumper`.
`extensions` are only supported by PostgreSQL database and postgresql database adapter.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment