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

index option added for change_table migrations #23593

Merged
merged 1 commit into from Oct 1, 2018

Conversation

@meinac
Copy link
Contributor

@meinac meinac commented Feb 10, 2016

In case if we want to add a column into the existing table
with index on it, we have to add column and index in two
seperate lines.
With this feature we don't need to write an extra line to
add index for column. We can just use index option.

Old behaviour in action:

  change_table(:languages) do |t|
    t.string :country_code
    t.index: :country_code
  end

New behaviour in action:

  change_table(:languages) do |t|
    t.string :country_code, index: true
  end

Exactly same behaviour is already exist for create_table migrations.
There was a pull request created by me which was about adding index option for add_column helper. I understand the concern behind closing it and it make sense but this addition is different from it. So I'm not pushing same idea again.

@rails-bot
Copy link

@rails-bot rails-bot commented Feb 10, 2016

r? @rafaelfranca

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

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Still relevant in Rails 6 (e925cb4). Approach looks good to me. Please rebase and fix the CHANGELOG conflict and we can look at getting this merged!

@meinac
Copy link
Contributor Author

@meinac meinac commented Sep 18, 2018

@rafaelfranca you were the one with objection on this feature, are you still against of this? If not, I'll resolve the conflicts and then we can merge. I remember too many times I tried to add columns to tables with indices by forgetting that rails does not support this feature and it's really annoying. WDYT?

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Sep 18, 2018

If there's still pushback, you could refactor this PR to raise an error if someone tries to add an index inline within a change_table block but considering you can add indices with t.index in change_table I see no problem with merging this.

Either way, there's something that needs to be added because its really unclear that this is currently unsupported.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Sep 21, 2018

I'm fine with it in column. Can you rebase?

In case if we want to add a column into the existing table
with index on it, we have to add column and index in two
seperate lines.
With this feature we don't need to write an extra line to
add index for column. We can just use `index` option.

Old behaviour in action:
```
  change_table(:languages) do |t|
    t.string :country_code
    t.index: :country_code
  end
```

New behaviour in action:
```
  change_table(:languages) do |t|
    t.string :country_code, index: true
  end
```

Exactly same behaviour is already exist for `create_table` migrations.
@meinac meinac force-pushed the meinac:add_index_option_for_change_table branch from 7ba022b to 5e4c22d Sep 22, 2018
@meinac
Copy link
Contributor Author

@meinac meinac commented Sep 22, 2018

@kamipo kamipo merged commit 5e4c22d into rails:master Oct 1, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kamipo added a commit that referenced this pull request Oct 1, 2018
index option added for change_table migrations
@meinac meinac deleted the meinac:add_index_option_for_change_table branch Oct 1, 2018
suketa added a commit to suketa/rails_sandbox that referenced this pull request Jun 3, 2019
index option added for change_table migrations
rails/rails#23593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.