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

use ActiveRecord::Migration version 5.0 or 5.1 instead of 4.2 #930

Closed
wants to merge 1 commit into from

Conversation

wayne5540
Copy link

@wayne5540 wayne5540 commented Dec 3, 2018

Why

  1. Since this gem is dropping support for Rails 4.2 (Rails 5.2 support, drop Rails 4.2 support. #887), there is no reason to use ActiveRecord::Migration[4.2] as default migration version, sooner we migrate to 5 better
  2. In Rails version > 5.1, default type of primary key becomes bigint, use ActiveRecord::Migration[5.1] can also help us adapt this change, make it more consistent

Notable point

  1. In Rails version > 5, migration method t.references will add index automatically, to keep migration result consistent, I add index: false for tag, taggable , tagger to prevent extra index_taggings_on_taggable_type_and_taggable_id and index_taggings_on_tagger_type_and_tagger_id index
  2. After this change, if your ActiveRecord version > 5.1, all table's id will become bigint (if your database supports bigint)

@wayne5540 wayne5540 force-pushed the use-rails5-migration branch from 73959bd to c0f3405 Compare December 3, 2018 02:15
@wayne5540 wayne5540 changed the title use ActiveRecord::Migration[5.0] instead of 4.2 version use ActiveRecord::Migration version 5.0 or 5.1 instead of 4.2 Dec 3, 2018
@wayne5540 wayne5540 force-pushed the use-rails5-migration branch from c0f3405 to 395922e Compare December 3, 2018 02:18
@wayne5540 wayne5540 force-pushed the use-rails5-migration branch from 395922e to 54e7ec3 Compare December 3, 2018 02:19
@nbulaj
Copy link
Contributor

nbulaj commented May 8, 2020

Why not just use [#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}] to support any AR version?

@wayne5540
Copy link
Author

@nbulaj Yes, agree with you, since .gemspec now has run time activerecord
dependency >= 5.0, < 6.1, we probably don't need the if else statement anymore and we can just use [#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}], thanks for pointing this out.

I'm happy to do the change, however, this PR has been idle here for a while, I'm not sure if this change is wanted 😅

@nbulaj
Copy link
Contributor

nbulaj commented May 13, 2020

I'm happy to do the change, however, this PR has been idle here for a while, I'm not sure if this change is wanted sweat_smile

You need to ask them, try to ping @seuros :)

@seuros seuros added this to the 9.0.0 milestone Nov 9, 2021
@seuros
Copy link
Collaborator

seuros commented Nov 9, 2021

@wayne5540 Thank you for your contribution, can you rebase this PR and have it use version 6.0 instead.

@wayne5540
Copy link
Author

wayne5540 commented Dec 27, 2021

Looks like all the files has been updated to 6.0 version at master branch, nice work team! This PR is no longer needed.

@wayne5540 wayne5540 closed this Dec 27, 2021
@wayne5540 wayne5540 deleted the use-rails5-migration branch December 27, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants