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

Output a warning when the index kwarg is passed to #add_column. #39834

Closed
wants to merge 1 commit into from

Conversation

d12
Copy link
Contributor

@d12 d12 commented Jul 14, 2020

Closes #39230

cc #20400
cc https://makandracards.com/makandra/32353-psa-index-true-in-rails-migrations-does-not-work-as-you-d-expect

Summary

The index: true kwarg works on many methods in ActiveRecord migrations but does not work in add_column. Rather than failing silently, provide a big quality of life improvement and let the user know that their migration did not create an index.

Other Information

I think a better long term solution would be to warn on all unexpected options keys but AR adapters could pass down random options keys that they use. Since solving perfectly is a little tricky, I'm pushing this PR up as a solid first step.

I also considered erroring on the index kwarg but we'll break backwards compatibility of old mutations with index kwargs.

Many cases of people mistakenly using index: true with add_column: https://github.com/search?q=%22add_column%22+%22index%3A+true%22&type=Code

cc rails#20400
cc rails#39230
cc https://makandracards.com/makandra/32353-psa-index-true-in-rails-migrations-does-not-work-as-you-d-expect

The `index: true` kwarg works on many methods in ActiveRecord migrations but does _not_ work in `add_column`. Rather than failing silently, provide a big QoL improvement and let the user know that their migration _did not_ create an index.
@d12 d12 force-pushed the warn_on_add_column_with_index_kwarg branch from f8c0577 to 9e76341 Compare July 14, 2020 00:20
@tgxworld
Copy link
Contributor

I think a better long term solution would be to warn on all unexpected options keys but AR adapters could pass down random options keys that they use.

I currently have a PR that tackles this #39659.

@rails-bot
Copy link

rails-bot bot commented Oct 12, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

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.

Migrations Should Raise an Exception for Unsupported Options
2 participants