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

The default arg of index_name_exists? makes to optional #26930

Merged
merged 2 commits into from Feb 13, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Oct 29, 2016

The default arg of index_name_exists? is only used the adapter does
not implemented indexes. But currently all adapters implemented
indexes (See #26688). Therefore the default arg is never used.

@rails-bot
Copy link

rails-bot commented Oct 29, 2016

r? @chancancode

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

# as there's no way to determine the correct answer in that case.
def index_name_exists?(table_name, index_name, default)
return default unless respond_to?(:indexes)
def index_name_exists?(table_name, index_name, default = nil)
Copy link
Member

@rafaelfranca rafaelfranca Oct 29, 2016

Choose a reason for hiding this comment

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

Can we deprecate this argument?

Copy link
Contributor

@pixeltrix pixeltrix Oct 30, 2016

Choose a reason for hiding this comment

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

Does anyone actually use this method externally? When I added the index_exists? (and column_exists?) methods in 11ff3da I renamed this method from index_exists? because it seemed to only be used internally at the time and was added as part of a general refactoring of add_index and remove_index in 99bcce7. I was wandering whether the whole method should be deprecated and made private?

@kamipo kamipo force-pushed the index_name_exists_default branch from 3951f70 to 16d2239 Compare Nov 3, 2016
def index_name_exists?(table_name, index_name, default = nil)
unless default.nil?
ActiveSupport::Deprecation.warn(<<-MSG.squish)
Passing default to #index_name_exists? is deprecated without replacement.
Copy link
Member Author

@kamipo kamipo Nov 3, 2016

Choose a reason for hiding this comment

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

I added the deprecation message.

@kamipo kamipo force-pushed the index_name_exists_default branch 2 times, most recently from 914ef90 to a5f2e4b Compare Nov 10, 2016
@kamipo kamipo force-pushed the index_name_exists_default branch 2 times, most recently from 80f1c9e to d31a104 Compare Nov 27, 2016
@kamipo kamipo force-pushed the index_name_exists_default branch from d31a104 to 70cfd76 Compare Nov 30, 2016
@kamipo kamipo force-pushed the index_name_exists_default branch from 70cfd76 to 272b1db Compare Dec 11, 2016
@kamipo kamipo force-pushed the index_name_exists_default branch from 272b1db to 830f196 Compare Dec 24, 2016
@kamipo kamipo force-pushed the index_name_exists_default branch 3 times, most recently from 3ba087f to d99ce24 Compare Jan 9, 2017
@kamipo kamipo force-pushed the index_name_exists_default branch from d99ce24 to 1e424b1 Compare Jan 16, 2017
@kamipo kamipo force-pushed the index_name_exists_default branch 2 times, most recently from 75eae94 to 2739378 Compare Feb 11, 2017
kamipo added 2 commits Feb 13, 2017
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.
@rafaelfranca rafaelfranca merged commit e0d0dc6 into rails:master Feb 13, 2017
2 checks passed
@kamipo kamipo deleted the index_name_exists_default branch Feb 13, 2017
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Feb 13, 2017
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Feb 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants