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.

Sign up for GitHub

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

Jump to bottom

Topic/producer postgresql batch alter table #44

Closed
wants to merge 1 commit into from
Closed

Topic/producer postgresql batch alter table #44

wants to merge 1 commit into from

Conversation

SysPete
Copy link
Contributor

@SysPete SysPete commented Aug 22, 2014

This initial version makes sure that drops on old table are handled before table rename and also fixes up rename_field/alter_field so that old field table name is changed to the name of the new table to prevent calls to alter_field from croaking. Test diffs are reordered but are otherwise unchanged.
I've added a test in a separate commit which uses Test::PostgreSQL to demonstrate one specific bug that this PR fixes (GH issue #40).

ilmari
ilmari reviewed Aug 22, 2014
View reviewed changes
t/postgresql-rename-table-and-field.t
use SQL::Translator::Diff;

eval "use DBD::Pg";
plan skip_all => "DBD::Pg required" if $@;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Test::SQL::Translator::maybe_plan() instead of hand-rolling this.

@SysPete
Copy link
Contributor Author

SysPete commented Aug 22, 2014

Good point - amazing how stupid I can be at times.

ilmari
ilmari reviewed Aug 22, 2014
View reviewed changes
t/postgresql-rename-table-and-field.t
foreach my $line (@diff) {
$line =~ s/\n//g;
next if $line =~ /^--/;
lives_ok { $dbh->do($line) || die } "$line";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of lives_ok { … || die }, set RaiseError (preferrably) or just use ok( … ).

@SysPete SysPete force-pushed the topic/producer_postgresql_batch_alter_table branch from 2fd7173 to 78c6262 Compare August 23, 2014 10:20
@SysPete
Copy link
Contributor Author

SysPete commented Aug 23, 2014

Initial test commit and later changes now merged into single commit.

@frioux
Copy link
Member

frioux commented Sep 1, 2014

I think you forgot to push your single commit version. Second your "fail" mode dies out poorly:

  $ perl -Ilib t/postgresql-rename-table-and-field.t
  1..10
  You tried to plan twice at t/postgresql-rename-table-and-field.t line 16.
  # Looks like your test exited with 2 before it could output anything

@SysPete SysPete force-pushed the topic/producer_postgresql_batch_alter_table branch from 78c6262 to 8bfb060 Compare September 1, 2014 08:26
@SysPete
Copy link
Contributor Author

SysPete commented Sep 1, 2014

Changes and new test folded into a single commit. Fail should now die cleanly.

@ilmari
Copy link
Member

ilmari commented Sep 1, 2014

I've reworked the tests a bit more and cleaned up the batch_alter_table code some more. Please see the people/ilmari/batch-alter branch (4ffe1f6).

@SysPete
Copy link
Contributor Author

SysPete commented Sep 1, 2014

Had a look at people/ilmari/batch-alter branch + tested my ::Versioned::Inline toy against it and all seems good. I guess this PR can be closed?

@ilmari ilmari closed this in 59aa003 Sep 2, 2014
@ilmari ilmari mentioned this pull request Jan 29, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SysPete @frioux @ilmari