Duplicate the given options from the migration instead of modifying in place

[lowjoel]

This allows the same options to be used by different parts of the migration script, e.g.

hash = { references: :users }
t.references :creator, foreign_key: hash
t.references :updater, foreign_key: hash

Which would be invalid otherwise.

[lowjoel]

get_fk_args can return a new hash, so it should return new hashes for all conditions, not just some, for consistency.

[ronen]

@lowjoel Nice idea to be able to reuse the hash. Looks like it's choking on non-hash shortcut values though. Maybe change to something like

args = column_options[:foreign_key]
args = args.dup if Hash === args

?

[lowjoel]

Take 2. Strange, I don't get emails when Travis fails... haha

And I've used is_a? over === since it seems like === should be reserved for case statements...

[ronen]

@lowjoel I guess I can act as Travis's email agent :)

And yeah, probably better to use is_a?. I forget when/why I got into the habit of using ===.

Feel like adding a spec for this case to make sure so nobody goes and deletes those dup's...?

[lowjoel]

Sounds like a good idea. Should have wrote a test just now.....

[lowjoel]

@ronen I can't figure out where to place the spec. hmm.

[lowjoel]

@ronen I've had a shot, let me know.

The second one, I realise, it useless. We use t.integer :column_id, references: nil and not the normal hash.

[ronen]

@lowjoel yeah that seems like a good place for the spec.

but should the spec be using t.references instead of t.integer, since that's the case that originally tripped you up?

The second one, I realise, it useless. We use t.integer :column_id, references: nil and not the normal hash.

not sure what you're referring to here... is that something you've already deleted from the PR?

[lowjoel]

yeah, I removed the second dup call.

t.references or t.integer shouldn't make a difference, I think. Both went to the same helper method, if I read things and debugged it right.

[ronen]

t.references does go through an extra code path which twiddles the options a bit, see https://github.com/SchemaPlus/schema_plus/blob/master/lib/schema_plus/active_record/connection_adapters/table_definition.rb#L109

maybe put both in the test -- one line with t.references and one with t.integer? Come to think of it, that'd help illustrate why the hash shouldn't change.

also, looking at that t.references code, it does do some option hacking for the :index. So maybe for a more robust test, include :index => :unique or some such in the hash?

[lowjoel]

interesting, I ddin't see that. I'll add those two tests in a bit.

[coveralls]

Coverage increased (+0.0%) when pulling 3421070960bb7ff179b346f40d15dd1dabedf4ca on lowjoel:master into d54072d7687be11faebd8669df4ac91bb04cf957 on SchemaPlus:master.

[lowjoel]

@ronen I've added a similar test for t.references. For :index what should we be testing? Does what I just pushed work?

[coveralls]

Coverage decreased (-2.36%) when pulling a344c23daf4c91bf4ca49639f2fe2214b0361c61 on lowjoel:master into d54072d7687be11faebd8669df4ac91bb04cf957 on SchemaPlus:master.

[coveralls]

Coverage decreased (-2.36%) when pulling a344c23daf4c91bf4ca49639f2fe2214b0361c61 on lowjoel:master into d54072d7687be11faebd8669df4ac91bb04cf957 on SchemaPlus:master.

[lowjoel]

@ronen I'm out of ideas as to where the index parameter can be modified...

[coveralls]

Coverage increased (+0.0%) when pulling 28e3abc93f7ca9ac629518c160573fd093827bb9 on lowjoel:master into d54072d7687be11faebd8669df4ac91bb04cf957 on SchemaPlus:master.

[coveralls]

Coverage increased (+0.0%) when pulling c6d90c3791c8a8d063adf5a8ffa7ab98fe099ba9 on lowjoel:master into d54072d7687be11faebd8669df4ac91bb04cf957 on SchemaPlus:master.

[ronen]

@lowjoel I cherry-picked this back to the 1.x branch, and released v1.8.3

thanks!