Set the primary key during #copy_table if necessary. Fixes [#2312] #6186
Conversation
|
@rafaelfranca here's the updated pull request for [#2312], as commented on here #2312 |
| @@ -522,7 +522,10 @@ def move_table(from, to, options = {}, &block) #:nodoc: | |||
| end | |||
|
|
|||
| def copy_table(from, to, options = {}) #:nodoc: | |||
| options = options.merge(:id => (!columns(from).detect{|c| c.name == 'id'}.nil? && 'id' == primary_key(from).to_s)) | |||
| options.merge!(:primary_key => primary_key(from)) if primary_key(from).to_s != 'id' | |||
There was a problem hiding this comment.
We don't need to use merge here. This should be fine
options[:primary_key] = primary_key(from) if primary_key(from).to_s != 'id'
|
Okay, making that change and retesting. I'll open a new pull request. |
|
@anydiem you don't need to open a new pull request. You can update this one pushing to the same branch. |
|
@rafaelfranca OK, I've made the requested changes, the old & new tests are still passing. How's this? |
| options[:primary_key] = primary_key(from) if primary_key(from).to_s != 'id' | ||
| unless options[:primary_key] | ||
| options[:id] = columns(from).detect{|c| c.name == 'id'}.present? && 'id' == primary_key(from).to_s | ||
| end |
There was a problem hiding this comment.
Perhaps we could cache the calls to primary_key(from)?
There was a problem hiding this comment.
Yes. This is a nice refactoring
|
@carlosantoniodasilva @rafaelfranca OK, that's been done too. How is it looking now? |
| options[:primary_key] = from_primary_key if from_primary_key != 'id' | ||
| unless options[:primary_key] | ||
| options[:id] = columns(from).detect{|c| c.name == 'id'}.present? && from_primary_key == 'id' | ||
| end |
There was a problem hiding this comment.
Just a question: before it was being tested for primary_key(from).to_s, now to_s is gone.. is that a problem or everything is working fine - ie all tests passing?
There was a problem hiding this comment.
Yes, all tests are passing.
I couldn't think of a good reason for to_s to remain. It's not necessary to guard against nil in a string comparison, and the column name should already be a string.
|
@carlosantoniodasilva @rafaelfranca Is there anything else that still needs doing on this request? Is it ready to be pulled? |
|
Seems good. Could you squash the commits in one? |
|
@rafaelfranca OK, done. How's that? |
|
Ok. I'll merge it later. |
|
@anydiem Thanks 👍 |
Set the primary key during #copy_table if necessary. Fixes [#2312]
Updated the pull request and retested against rails:master, as requested