Refactor handlers as roles

[yanick]

This is more of a Request For Comments rather than a PR. If you like what you see, I'll edit the documentation and clean up the code before I make a final PR.

Basically, I saw if I could simplify a little bit the structure of DBDH. What I did is to turn the objects implementing Handles(Deploy|Versioning|VersionStorage) into role, and I apply those to the main class as build time. This removes the need for the ::Dad class, and the juggling done with ::WithDumple.

E.g., to get a DH object with the default handlers and the WithReasonableDefault:

my $dh = DBIx::Class::DeploymentHandler->new( schema => $schema );

to use a different versioning handler:

my $dh = DBIx::Class::DeploymentHandler->new( 
  schema => $schema,
  version_handler_class => 'DBIx::Class::DeploymentHandler::Versioning::Random', 
);

The only really gnarly bit of the new code is the class morphing I do in new: if the class doesn't already consume the 3 Handles roles, I create a new class that does, and transparently create an object of that new class.

Anyway, let me know if that's something that appeal to you. :-)

[frioux]

I understand your POV, but that stuff is structured that way on purpose. The idea is that the smaller objects can be tested in isolation without all the rest of it. The structure of the code should be such that if you want you can slot in a new object, or if you'd rather you can just subclass the big thing and override methods.

I'll leave this open for a while in case I realize something else, but as it stands I think accepting this PR would not really be an improvement.

[yanick]

More than fair enough. And I know it's a rather radical change, hence the cautious RFC I went too far.

But, if you allow me, just for the sake of discussion I'll put on my used car saleman's outfit and try to sell you how this change isn't as bad as you think.

The idea is that the smaller objects can be tested in isolation without all the rest of it.

This is still possible. Where before you would do

use DBIx::Class::DeploymentHandler::DeployMethod::SQL::Translator;

my $schema = ...;

my $deploy_handler = DBIx::Class::DeploymentHandler::DeployMethod::SQL::Translator->new(
    schema => $schema,
);

now you would do

use DBIx::Class::DeploymentHandler::DeployMethod::SQL::Translator;

{
    package My::TestDH;
    use Moose;
    with 'DBIx::Class::DeploymentHandler::DeployMethod::SQL::Translator';
}

my $schema;

my $deploy_handler = My::TestDH->new(
    schema => $schema,
);

It's a few more lines, granted, but that added complexity for the test cases is counter-balanced for the simplification of the common case (see below).

The structure of the code should be such that if you want you can slot in a new object, or if you'd rather you can just subclass the big thing and override methods.

That is indeed the strength of DBIC::DH, and I think it's even simplified using roles. :roll up sleeves: Let me demonstrate.

So we have the vanilla-flavored case that uses all the common defaults:

my $dh = DBIx::Class::DeploymentHandler->new(
    %usual_args
);

We want to use explicit versions instead of the default monotonic behavior?

my $dh = DBIx::Class::DeploymentHandler->new(
    version_handler_class => 'DBIC::DH::VersionHandler::Explicit'
    %usual_args
);

We want to use a custom version handler that increments in a different way?

{
    package My::Version;

    use Moose::Role;
    with 'DBIx::Class::DeploymentHandler::VersionHandler::Monotonic';

    sub _inc_version {
        $_[0]->_version($_[0]->_version + 2 ) 
    }
}

my $dh = DBIx::Class::DeploymentHandler->new(
    version_handler_class => 'My::Version'
    %usual_args
);

Don't want ReasonableDefaults but our own WackyDefaults?

my $dh = DBIx::Class::DeploymentHandler->new(
    additional_classes => [
        'DBIx::Class::DeploymentHandler::WithWackyDefaults',
    ],
    %usual_args
);

Want to throw in one more role that does something wildly different? No prob:

my $dh = DBIx::Class::DeploymentHandler->new(
    additional_classes => [
        'DBIx::Class::DeploymentHandler::WithWackyDefaults',
        'DBIx::Class::DeploymentHandler::SyncWithGit',
    ],
    %usual_args
);

So we can still mix-and-match all the handlers and throw in new overrides as we please. But now we can do it without having to create a custom base class ourselves. Hmm... I just realized that I'm not covering the case where we /do/ want to create a base class with all our customized defaults. But fear not, I think I already know how to do that.

For the record, I spend a fair amount of time debating sub-objects or roles. In this case, it's my (very personal, thus prone to be very wrong) opinion that roles fit a little better, as the functionality of the different aspects are fairly tightly coupled. Having them all be roles to the main class makes the accessibility of the parameters of one aspect to the others much easier: you don't have to worry about passing them explicitly, but rather just requires them by the consumer and let the role logic yell at you if there's nothing that provide that method.

In all cases, I've been probably been babbling for too long already. I just needed to let the voices in my head come out. :-)

[frioux]

I'm gonna close this, just to clean stuff up. Thanks for this though; sorry it didn't work out

[yanick]

No problem. You win some, you lose some. Such is the PR game. :-)