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

Add reversible syntax for change_column_default #20018

Merged
merged 2 commits into from Jun 27, 2015

Conversation

Projects
None yet
4 participants
@sikachu
Copy link
Member

commented May 4, 2015

Passing :from and :to to change_column_default makes this command
reversible as user has defined its previous state.

So, instead of having the migration command as:

change_column_default(:posts, :state, "draft")

They can write it as:

change_column_default(:posts, :state, from: nil, to: "draft")

This also includes a change to guide to reflect this.

change_column_default :products, :approved, from: true, to: false
```

TIP: `change_column` command is always irreversible, `change_column_null` is

This comment has been minimized.

Copy link
@egilburg

egilburg May 5, 2015

Contributor

IMO this tip is redundant especially with so many clauses, should be enough to just clearly document reversibility of each command in that command's description.

@@ -436,8 +436,17 @@ change_column_default :products, :approved, false
This sets `:name` field on products to a `NOT NULL` column and the default
value of the `:approved` field to false.

TIP: Unlike `change_column` (and `change_column_default`), `change_column_null`
is reversible.
Additionaly, for `change_column_default`, you can pass a Hash which defines the

This comment has been minimized.

Copy link
@egilburg

egilburg May 5, 2015

Contributor

We should encourage writing reversible migrations when possible. Let's change default documentation to use from: and to:, and here write something like:

Note: You could also write the above migration as change_column_default :products, :approved, false, but unlike the previous example, this would make your migration irreversible.

This comment has been minimized.

Copy link
@sikachu

sikachu May 6, 2015

Author Member

Oh, I like that. Let me update the doc.

```

TIP: `change_column` command is always irreversible, `change_column_null` is
always reversible, and `change_column_default` is only reversible if using

This comment has been minimized.

Copy link
@egilburg

egilburg May 5, 2015

Contributor

Not in scope of this PR, but would be nice if we could extract keys from change_column command, and normalize them to reversible migrations as long as all keys are reversible. E.g. combining null and default often goes together in a migration that sets null to false:

change_column 'foo', null: false, default: { from: nil, to: 'bar' }

Would be interpreted as:

change_column_default 'foo', from: nil, to: 'bar'
change_column_null 'foo', false

Above order is important, if null is disallowed the default must be changed first, if null is allowed the default must change last.

This comment has been minimized.

Copy link
@sikachu

sikachu Jun 26, 2015

Author Member

Noted. I'm going to submit another PR for it.

def invert_change_column_default(args)
table, column, options = *args

unless options && options.is_a?(Hash) && options[:from] && options[:to]

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva May 7, 2015

Member

Wouldn't this fail if from or to are nil/false?

This comment has been minimized.

Copy link
@sikachu

sikachu May 7, 2015

Author Member

Good question. I would expect that it should, but the test didn't fail. Let me look further into this.

This comment has been minimized.

Copy link
@carlosantoniodasilva

carlosantoniodasilva May 7, 2015

Member

That might be because the test uses strings, try adding one more to test booleans just in case :)

This comment has been minimized.

Copy link
@sikachu

sikachu Jun 26, 2015

Author Member

Yep, it failed indeed. Good catch.

I'm going to push an update to this.

@sikachu sikachu force-pushed the sikachu:change-column-default-recorder branch 2 times, most recently from 71221b0 to 2657382 Jun 26, 2015

sikachu added some commits May 4, 2015

Add reversible syntax for change_column_default
Passing `:from` and `:to` to `change_column_default` makes this command
reversible as user has defined its previous state.

So, instead of having the migration command as:

    change_column_default(:posts, :state, "draft")

They can write it as:

    change_column_default(:posts, :state, from: nil, to: "draft")

@sikachu sikachu force-pushed the sikachu:change-column-default-recorder branch from 2657382 to f9c8419 Jun 26, 2015

@sikachu

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2015

@egilburg @carlosantoniodasilva would you mind give this PR another look and maybe merge it? Thanks!

rafaelfranca added a commit that referenced this pull request Jun 27, 2015

Merge pull request #20018 from sikachu/change-column-default-recorder
Add reversible syntax for change_column_default

@rafaelfranca rafaelfranca merged commit 859c8c3 into rails:master Jun 27, 2015

@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Jun 27, 2015

Awesome! Thank you @sikachu

@sikachu sikachu deleted the sikachu:change-column-default-recorder branch Jun 27, 2015

yui-knk added a commit to yui-knk/rails that referenced this pull request Aug 25, 2015

Make `change_column_default` to work
This is fix of rails#20018 which removes `change_column_default` from
array, so `CommandRecorder#method_missing` catches
`change_column_default` and @DeleGate's method is called.

This PR

* fix this bug
* define `ReversibleAndIrreversibleMethods` const making clear
  which this array means to prevent these miss

yui-knk added a commit to yui-knk/rails that referenced this pull request Aug 31, 2015

[ci skip] Update what methods `Migration#change` can reverse
* Documentations and comments about what methods
  `Migration#change` can reverse is out of date.
  For example `change_column_default` is now reversible
  by this [commit](rails#20018).

* Comments about `CommandRecorder` dose not match with Rails Guide.
  For example `add_foreign_key` is listed only on Rails Guide.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request May 17, 2016

yahonda added a commit to yahonda/activerecord-jdbc-adapter that referenced this pull request Oct 11, 2016

y-yagi added a commit to y-yagi/rails that referenced this pull request Oct 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.