frioux / DBIx-Class-DeploymentHandler

https://metacpan.org/pod/DBIx::Class::DeploymentHandler
20 stars 25 forks source link

Result override #57

Closed andrewgregory closed 6 years ago

andrewgregory commented 6 years ago

This patchset is meant to achieve four things:

The reason for this is that I have multiple projects with their own individual schemas that I now need to be able to use together. Because I used the default table name for the first one, I need to both tell DBICDH to use a custom table (and index) name and update the schema for the existing database table. I also need to set a custom source name so that they can all co-exist within a single DBIC schema instance.

I have not yet made any changes to the documentation because I wanted to get feedback on whether or not the changes would be acceptable first.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.004%) to 86.058% when pulling 884ed10a81a2a0ecae726fbdb16e12de1a78b9c1 on andrewgregory:result-override into 0c28f0112d95b193c8da3c4524917282d16982ca on frioux:master.

frioux commented 6 years ago

There are docs on how to do this with the current code, for what it's worth, but I think the added configurables are reasonable. I haven't done an in depth code review but on a quick skim it seems fine.

andrewgregory commented 6 years ago

I was actually aware of the cookbook doc, but overriding DeploymentHandler is non-trivial and not something I want to have to do for each individual project that uses DBICDH. I've updated the PR with additional tests and a commit removing the now defunct cookbook page. I couldn't find a good place to document the new options (initial_version, version_class, and version_storage). If you can point me to the correct place to document those, I'll update the PR.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.004%) to 86.058% when pulling 5628713908b5205f8e583675cb86ce52656e3f5c on andrewgregory:result-override into 0c28f0112d95b193c8da3c4524917282d16982ca on frioux:master.

andrewgregory commented 6 years ago

Ping. I've got a pending update that needs these changes. Any chance that we can get this merged in the next few days? Otherwise I'm going to have to start maintaining a private fork.

frioux commented 6 years ago

Sorry for the delay, I will probably release this (and another outstanding PR) tomorrow when I get to work.

andrewgregory commented 6 years ago

Thanks.

frioux commented 6 years ago

Ok so looking at doc stuff, I'd at add version_source to ::DM::SQL::Translator.

I don't exactly understand the reasoning behind renaming database_version to initial_version (the idea is that you start at the database_version and upgrade to the current schema_version.) Is that important for this patch? It seems like a naming change that doesn't really add any functionality.

andrewgregory commented 6 years ago

Renaming database_version to initial_version is to allow skipping the initial read from the database to allow renaming the version table. The VersionHandler needs the starting version and database_version is always read directly from the database itself, so there's no way to have it pull the initial version from the original table but then add the new version to the renamed table at the end. Given how DH passes attributes between its components, renaming the VersionHandler's database_version was the only way I could figure out to decouple it from the VersionStorage's database_version.

frioux commented 6 years ago

Ok I'll buy it.

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.004%) to 86.058% when pulling 93200c27623e9a99004dc447d2a613d9ae9b4457 on andrewgregory:result-override into 0c28f0112d95b193c8da3c4524917282d16982ca on frioux:master.

frioux commented 6 years ago

I have not forgotten this, just working on making this less of a delay in the future.

mmcclimon commented 6 years ago

I'm part of the the solution to "making this less of a delay in the future"; this PR looks good, and I'll try to cut a release tomorrow!

mmcclimon commented 6 years ago

As it turns out, I had some time tonight...I just cut v0.002221, with this set of commits in it. Thanks!