Adding the ability to rename columns #39
Conversation
|
This is the last one, I promise. :) |
|
|
||
| definition = col[:type] | ||
| definition += " NOT NULL" unless col[:is_nullable] | ||
| definition += " DEFAULT #{col[:column_default]}" if col[:column_default] |
There was a problem hiding this comment.
Can you make sure this line is covered by the tests?
There was a problem hiding this comment.
I actually finished that on the plane on Tuesday. As it turns out the code as written is wrong. <gasp!>
I’m at a company retreat for the next few days, but I’ll try to get that and the build failures fixed as soon as I have a free moment.
|
Looks good! |
|
And the builds needs work :) |
|
The ActiveRecord failure is a timing issue, and the DM failures might be related to f821ce2#L1L54 |
|
👍 Needs a rebase though, the branch conflicts with the recently added conditional migrations. |
|
I just rebased off the latest master (squashing the fix into the original commit in the process). This fixes the conflicts with conditional-migrations. Sorry for the delay! |
|
Always good to make sure you haven’t switched branches before running tests. Oops. |
...And keep the data in them. Use the adapter to quote the value.
|
Bump. @lgierth is there anything else that needs to be done before this PR is merged? |
|
Looks good to me, but I left SoundCloud and can't merge. @pellegrino @durran WDYT? |
|
@pellegrino @durran anyone wanna merge this? It'll be useful. |
|
I'd be interested in that as well. Any updates on this? |
|
+1 for this. |
|
+1 for this |
|
I will review this today, and merge it up... thanks for the patch, and sorry for the delay |
|
trying to merge this, but I am getting a local test falling.. Could you rebase and as soon as tests are 💚 I will |
..
|
@erikogan can it be rebased and merged? |
|
I barely remember these changes now, but I’ll rebase/merge (depending on conflicts) the latest master and see if I can see what’s going on with the tests. |
Conflicts: lib/lhm/chunker.rb spec/integration/lhm_spec.rb
…ourne shell, the ~ construct does not work.
…on file to live somewhere other than the user home directory
This is a rare corner case, but it was sending me down the rabbit hole in spec testing.
|
Along the way I did a minor refactor of the spec setup scripts in bin/ In retrospect that should probably have been a separate branch, to be evaluated out of this context. If they end up being controversial in any way I can break them out. I also merged my other pull-request into this branch because it was discovered trying to get these specs to pass, and WAS a separate issue. |
|
@arthurnn Any luck merging this? I would hate for us to diverge again. |
|
I will take a final look on this. thanks |
|
@arthurnn Any further progress on merging this? |
|
Any update on this PR? Would be good to get it merged. |
|
Looks good to me. @arthurnn Let's merge? |
Adding the ability to rename columns
|
great.. thanks for this PR.. ❤️ |
| @@ -25,7 +25,7 @@ def initialize(migration, connection = nil, options = {}) | |||
| def execute | |||
| return unless @start && @limit | |||
| @next_to_insert = @start | |||
| until @next_to_insert >= @limit | |||
| while @next_to_insert < @limit || (@next_to_insert == 1 && @start == 1) | |||
There was a problem hiding this comment.
was the assumption here that the id of a table with a single record was always 1?
...And keep the data in them.
I tried to keep the most invasive changes within the Intersection class.