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.

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

fix Producer::SQLite::batch_alter_table rename field #39

Merged
merged 1 commit into from
Aug 20, 2014

Conversation

SysPete
Copy link
Contributor

@SysPete SysPete commented Jun 18, 2014

rename_field is ignored by Producer::SQLite and the existing tests have:

SELECT ... physical_description FROM person"

which should be:

SELECT ... description FROM person"

Patches address this and try to make some of the code (mostly inside batch_alter_table) more readable.

@SysPete SysPete closed this Jul 1, 2014
@SysPete
Copy link
Contributor Author

SysPete commented Jul 3, 2014

Further testing with my new DBIC distro ::Versioned::Inline has proved that this is a real bug so reopening. Will hopefully have time in the next few days to add some useful test to prove this.

@SysPete SysPete reopened this Jul 3, 2014
@SysPete
Copy link
Contributor Author

SysPete commented Jul 4, 2014

Further explanation on the issue:

SQLT::Diff works fine for SQLite with column renames as long as there are no rows in the table and produces perfectly good SQL for creating the replacement table. The problem is that the code for moving the data from old to new table is completely broken. That is what this patch set tries to fix.

@frioux
Copy link
Member

frioux commented Jul 5, 2014

Could you write a test that fails before the patch and passes after? That would help.

@SysPete
Copy link
Contributor Author

SysPete commented Jul 21, 2014

I've rehacked this into a single commit + supplied test that fails against master.

@SysPete SysPete force-pushed the fixes/sqlite_rename_field branch 4 times, most recently from 55e4835 to 05c9b80 Compare August 20, 2014 10:51
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) when pulling 55e4835 on SysPete:fixes/sqlite_rename_field into cb9bbc6 on dbsrgits:master.

@SysPete
Copy link
Contributor Author

SysPete commented Aug 20, 2014

I've removed some unnecessary changes from the diff which makes it much cleaner and rebased against master. I'm using this updated batch_alter_table method botched into DBIx::Class::Schema::Versioned::Inline to fixup SQLite upgrade problems which adds quite a few more tests in addition to the SQLT ones and all is well there.
Any chance of seeing this in the next release?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 05c9b80 on SysPete:fixes/sqlite_rename_field into cb9bbc6 on dbsrgits:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 05c9b80 on SysPete:fixes/sqlite_rename_field into cb9bbc6 on dbsrgits:master.

@dbsrgits-sync
Copy link

Will look at this later today I hope to see if we can get it merged.
Thanks!

-frew
(sent from my HTC 5c Galaxy megaphone, please pardon my brevity)

# record renamed fields for later
my %rename_field;
if ( @{$diffs->{rename_field}} ) {
foreach my $rf_diff ( @{$diffs->{rename_field}} ) {
Copy link
Member

Choose a reason for hiding this comment

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

No need for the if here, the for loop will just execute zero times if the array is empty.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 768f5d9 on SysPete:fixes/sqlite_rename_field into cb9bbc6 on dbsrgits:master.

@SysPete
Copy link
Contributor Author

SysPete commented Aug 20, 2014

Unnecessary ifs removed and loops replaced with map and delete hash slice.

@SysPete
Copy link
Contributor Author

SysPete commented Aug 20, 2014

ilmari> SysPete: I just noticed, you're quoting $temp_table_name twice
fixed - my bad.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 0ce3181 on SysPete:fixes/sqlite_rename_field into cb9bbc6 on dbsrgits:master.

@dbsrgits-sync dbsrgits-sync merged commit 0ce3181 into dbsrgits:master Aug 20, 2014
@SysPete SysPete deleted the fixes/sqlite_rename_field branch August 20, 2014 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants