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

Remove unnecessary `respond_to?(:indexes)` checking #26688

Merged
merged 1 commit into from Oct 28, 2016

Conversation

Projects
None yet
4 participants
@kamipo
Member

kamipo commented Oct 2, 2016

Currently all adapters (postgresql, mysql2, sqlite3, oracle-enhanced,
and sqlserver) implemented indexes and schema dumper expects
implemented indexes.

https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/schema_dumper.rb#L208

Therefore respond_to?(:indexes) checking is unnecessary.

Remove unnecessary `respond_to?(:indexes)` checking
Currently all adapters (postgresql, mysql2, sqlite3, oracle-enhanced,
and sqlserver) implemented `indexes` and schema dumper expects
implemented `indexes`.

https://github.com/rails/rails/blob/v5.0.0/activerecord/lib/active_record/schema_dumper.rb#L208

Therefore `respond_to?(:indexes)` checking is unnecessary.
@rails-bot

This comment has been minimized.

rails-bot commented Oct 2, 2016

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@kaspth

This comment has been minimized.

Member

kaspth commented Oct 3, 2016

All our built in adapters may support indexes but that's not guaranteed for other adapters like the Oracle one. Is indexes part of our public API?

Closing for now :)

@kaspth kaspth closed this Oct 3, 2016

@kamipo

This comment has been minimized.

@kaspth

This comment has been minimized.

Member

kaspth commented Oct 6, 2016

Sure, but what about yet other adapters? Not sure how long our backwards compatibility support cycle is for adapters, but I think we need a changelog entry so they know we now require them to implement indexes.

@kaspth kaspth reopened this Oct 6, 2016

@kaspth kaspth merged commit 86bf5ad into rails:master Oct 28, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Member

kaspth commented Oct 28, 2016

Sweet, then!

@kamipo kamipo deleted the kamipo:remove_respond_to_indexes branch Oct 28, 2016

kamipo added a commit to kamipo/rails that referenced this pull request Feb 13, 2017

The `default` arg of `index_name_exists?` makes to optional
The `default` arg of `index_name_exists?` is only used the adapter does
not implemented `indexes`. But currently all adapters implemented
`indexes` (See rails#26688). Therefore the `default` arg is never used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment