API is somewhat confusing

[mlawren]

Given the lack of discussion forum I hope writing this as an issue is ok. It is a bit of a shame that gtihub doesn't let you open an issue by email. At least not that I could find.

I have a couple of issues with the DBIx::Connector API.

The first is the use of the name "do()" in the context of a DBIx module. For coders writing DBI-related scripts a do() method has always meant "run this SQL against the database". The DBIx::Connector do() method means something different - "run this CODE, and run it again if database not connected". This is quite confusing. I've spent a good hour looking at integrating DBIx::Connecter into just one section of my code, and this similarity of method names for different meanings keeps tripping me up. Two times do() in three lines...

$conn->do(sub {
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');

The second issue I have is that the API doesn't help us move away from eval / if ($@) constructs (which are apparently a difficult thing to get right). I generally want to know or take an action if operations have succeeded or not, and this fast gets noisy and nested:

eval {
    $conn->txn_do(sub {
        my $dbh = shift;
        $dbh->do('INSERT INTO table1 VALUES (1)');
        eval {
            $conn->svp_do(sub {
                shift->do('INSERT INTO table1 VALUES (2)');
                die 'OMGWTF?';
            });
        };
        if ( $@ ) {
            warn "Savepoint failed\n";
            # Handle this
        }
        $dbh->do('INSERT INTO table1 VALUES (3)');
    });
};

if ( $@ ) {
    # Handle this
}

Taking a page from the Try::Tiny module (and maybe using Return::Value?) wouldn't something like the following be cleaner? By the way, prototyping txn_do() with coderef would also be nice to avoide the "sub":

$conn->try({
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
})->catch({
    # Handle the error
});

I'm thinking that in the event that the try() sub dies, the rollback happens and returns an object whos catch() method is run. If the try does not die the (other) returned object's catch() method does nothing.

Here's my idea of the long version:

$conn->txn({
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');

    $conn->svp({
        shift->do('INSERT INTO table1 VALUES (2)');
        die 'OMGWTF?';
    })->catch({
        warn "Savepoint failed\n";
        # Handle this
    });

    $dbh->do('INSERT INTO table1 VALUES (3)');
})->catch({
    # Handle this
});

The use of txn(), svp() would mean backwards compatability could be kept with txn_do...

Thoughts?

Cheers, Mark.

[mlawren]

Or to avoid mucking about with objects how about this alternative:

$conn->try({
    my $dbh = shift;
    $dbh->do('INSERT INTO table1 VALUES (1)');
}, catch {
    # Handle the error
});

[mlawren]

Even better - The "catch" subroutine could be optional, and doesn't even have to come from DBIx::Connector. It could be whatever TryCatch, Try::Tiny, or my_custom_catcher() subroutine the caller wants to use.

[mlawren]

Ok, so "catch" is generally a no-op... Please ignore the previous.

[theory]

I like the idea of catch(). I'll have to think about implementation, as I don't want to require that the user use it.

Prototyping doesn't work with method calls, alas. But you can use PerlX::MethodCallWithBlock.

—Theory

[theory]

Based on the discussion in this DBIx::Class ticket, I think I'm going to deprecate the do methods and add these methods, instead:

• run
• runup
• txn_run
• txn_runup
• svp_run

All will get the connection object as the argument, rather than the database handle. The runup methods will offer the optimistic connection stuff, while the run methods will ping.

—Theory

[theory]

Still pondering better names. Check out my blog entry on the topic.

—Theory

[theory]

Okay, 0.20 is on its way to the CPAN now with run, txn, and svp methods, which should eliminate the confusion with do. I'll do some thinking about exception catching next.

—Theory

[theory]

Mark,

What if I passed errors on to the DBI's HandleError attribute, it it's available?

—Theory

[theory]

Ignore that last comment, it was irrelevant.

I could simply make a rule that, if the argument passed after the block is also a block, that it will handle any exceptions. This would allow you to do something like this:

use Try::Tiny;

$conn->run( sub {
    my $dbh = shift;
    # ....
}, catch {
    warn "caught error: $_";
});

That is, catch is just syntactic sugar for sub, which you could also use. Indeed, any Try/Catch module that basically makes its catch return a code block (as Try::Tiny's does) could then be used there.

The only downside to this plan that I can think of is that, since other arguments to the run(), txn(), and svp() methods are passed through to the execution block, if you wanted to pass a code ref, you couldn't unless you also specified an exception handler. Maybe that's not such a bad thing, though.

Thoughts?

—Theory

[theory]

Okay, check out 24e6035484f011ea6b36027b893a8200c14ef4e8, which adds exception handling with just a second code ref passed to an execution method. Read the POD I added to describe it (search for "head3 Exception Handling") for how it works and how to take advantage of Try::Tiny at the same time.

I may or may not add our own exported catch function; I have not yet decided.

—Theory

[theory]

Maybe exception handlers should be specified by a keyword?

$conn->run(sub {
    die 'WTF!';
}, catch => sub {
    warn "Died: $_";
});

Just fiddling with ideas…

—Theory

[theory]

I've now released DBIx::Connector to CPAN. You can do this:

$conn->run(sub {
    die 'WTF!';
}, catch => sub {
    warn "Died: $_";
});

Or use it with Try::Tiny, like so:

use Try::Tiny;
$conn->run(sub {
    die 'WTF!';
}, catch {
    warn "Died: $_";
});

I'm pretty happy with this.

—Theory