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.

Sign up for GitHub

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

Jump to bottom

Pare back default index option for the migration generator #23179

Merged
merged 1 commit into from
Jan 24, 2016
Merged

Pare back default index option for the migration generator #23179

merged 1 commit into from
Jan 24, 2016

Conversation

prathamesh-sonpatki
Copy link
Member

r? @sgrif cc @rafaelfranca

@prathamesh-sonpatki prathamesh-sonpatki added this to the 5.0.0 milestone Jan 22, 2016
@vipulnsward
Copy link
Member

I believe, this will need a CHANGELOG.

@prathamesh-sonpatki
Copy link
Member Author

Yeah, I will add it also looking at failures from railties.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the pare-back-default-index-option-to-references branch 2 times, most recently from 454ffe9 to 71b4c05 Compare January 22, 2016 12:57
@prathamesh-sonpatki prathamesh-sonpatki changed the title references will add index by default now, user can index: false to opt out Pare back default index option for the migration generator Jan 22, 2016
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the pare-back-default-index-option-to-references branch from 71b4c05 to bc82a72 Compare January 22, 2016 13:18
prathamesh-sonpatki
prathamesh-sonpatki reviewed Jan 22, 2016
View reviewed changes
railties/test/generators/model_generator_test.rb
end
end
end

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed these tests. Should I add them for checking index_is_not_added_for_references_association ?

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the pare-back-default-index-option-to-references branch from bc82a72 to c3fb781 Compare January 23, 2016 17:29
@prathamesh-sonpatki
Copy link
Member Author

Changelog added and build is green now.

@sgrif
Copy link
Contributor

sgrif commented Jan 24, 2016

r? @matthewd IIRC you were involved in the versioned migration changes. That's landed, correct? I'm not familiar enough with what's happening there to know if this only affects new migrations or if it affects old as well. If we haven't finished versioned migrations, and this is changing the behavior of existing migrations, I feel quite strongly that this should wait until 5.1

@rails-bot rails-bot assigned matthewd and unassigned sgrif Jan 24, 2016
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the pare-back-default-index-option-to-references branch from c3fb781 to 7a028be Compare January 24, 2016 04:31
@prathamesh-sonpatki
Copy link
Member Author

@sgrif Yup, we have to update https://github.com/rails/rails/blob/master/activerecord/lib/active_record/migration/compatibility.rb to not create index by default unless specified for legacy migrations. I will work on it.

@matthewd
Copy link
Member

@prathamesh-sonpatki matthewd@f87c457 may be helpful... I had the Actual Change, but hadn't gone back and chased it through the tests

@prathamesh-sonpatki
Copy link
Member Author

Thanks, I will check it.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the pare-back-default-index-option-to-references branch 2 times, most recently from cb871a9 to 9f85bb5 Compare January 24, 2016 12:49
@prathamesh-sonpatki
Pare back default index option for the migration generator
909818b 
- Using `references` or `belongs_to` in migrations will always add index
  for the referenced column by default, without adding `index:true` option
  to generated migration file.
- Users can opt out of this by passing `index: false`.
- Legacy migrations won't be affected by this change. They will continue
  to run as they were before.
- Fixes rails#18146
@prathamesh-sonpatki prathamesh-sonpatki force-pushed the pare-back-default-index-option-to-references branch from 9f85bb5 to 909818b Compare January 24, 2016 13:18
@prathamesh-sonpatki
Copy link
Member Author

@matthewd Please review, took care of legacy migrations with your commit and added a test case for it.

matthewd added a commit that referenced this pull request Jan 24, 2016
@matthewd
Merge pull request #23179 from prathamesh-sonpatki/pare-back-default-…
235c759 
…index-option-to-references

Pare back default `index` option for the migration generator
@matthewd matthewd merged commit 235c759 into rails:master Jan 24, 2016
@matthewd
Copy link
Member

❤️

@prathamesh-sonpatki prathamesh-sonpatki deleted the pare-back-default-index-option-to-references branch January 24, 2016 13:59
@CodingItWrong CodingItWrong mentioned this pull request Jul 16, 2016
Merged
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jul 16, 2016
@prathamesh-sonpatki
Fix wrong test name
4c9f8ff 
- Followup of rails#23179
@prathamesh-sonpatki prathamesh-sonpatki mentioned this pull request Jul 16, 2016
Merged
prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Aug 17, 2016
@prathamesh-sonpatki
Fix documentation of index option for the add_reference method [c…
c451629 
…i skip]

- Followup of rails#23179.
@prathamesh-sonpatki prathamesh-sonpatki mentioned this pull request Aug 17, 2016
Merged
abicky added a commit to abicky/ridgepole that referenced this pull request Aug 19, 2017
@abicky
Add an index to the referencing column by default
787a984 
The default value of `index` is true from Rails 5.0.
cf. rails/rails#23179
abicky added a commit to abicky/ridgepole that referenced this pull request Aug 19, 2017
@abicky
Add an index to the referencing column by default
fe239cf 
The default value of `index` is true from Rails 5.0.
cf. rails/rails#23179
abicky added a commit to abicky/ridgepole that referenced this pull request Aug 19, 2017
@abicky
Add an index to the referencing column by default
521f4bc 
The default value of `index` is true since Rails 5.0.
cf. rails/rails#23179
@abicky abicky mentioned this pull request Aug 19, 2017
Merged
@kamipo kamipo mentioned this pull request Oct 17, 2018
Merged
adamkiczula added a commit to adamkiczula/public_activity that referenced this pull request Apr 29, 2023
@adamkiczula
Removes duplicate indexes from activities migration template
523de07 
As of rails/rails#23179, using `belongs_to` or `references` automatically adds an index. public-activity#64 previously manually added the indexes, but they are no longer needed.
@adamkiczula adamkiczula mentioned this pull request Apr 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@matthewd matthewd

Labels
activerecord railties
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@prathamesh-sonpatki @vipulnsward @sgrif @matthewd