-
Notifications
You must be signed in to change notification settings - Fork 189
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure this line is covered by the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.