Discrepancies in docs and implementation

[prev] [thread] [next] [Date index for 2004/10/08]

Regarding the update method, the docs say:

    If any columns have been updated then the "after_update" trigger is
    invoked after the database update has executed and is passed: ($self,
    discard_columns => \@discard_columns, rows => $rows)
    (where rows is the return value from the DBI execute() method).

but the code reads:

        $self->call_trigger('after_update', discard_columns => \@changed_cols);

"rows" isn't being passed along.

Also, the docs say:

    The update() method returns the number of rows updated, which should
    always be 1, or else -1 if no update was needed. ...

but the code reads:

        return 1 unless my @changed_cols = $self->is_changed;

It should be:

        return -1 unless my @changed_cols = $self->is_changed;

Further, the phrase "if no update was needed" could reasonably be
taken to mean "if the database record was the same as the object's
contents, and an SQL UPDATE was unnecessary"; but the phrase really
means "if no set accessors have been used on the object".

For example, given the "-1 patch" above, this code:
    $cd = Music::CD->retrieve(1);
    # no set accessors
    $rows_updated = $cd->update;
    print $rows_updated, "\n";
    # now, set to what's already there
    $cd->set(year => $obj->get('year'));
    $rows_updated = $cd->update;
    print $rows_updated, "\n";
will produce the following output:
-1
1

Some would have expected two -1's, but the update() method results in
an SQL UPDATE even if the values of the object's attributes have not
been changed.

No doubt, these semantics are obvious to cdbi experts, but the docs
should assume less expertise.  Perhaps the docs could be adjusted to
something similar to the following.

    The update() method returns the number of rows updated, which should
    always be 1, or else -1 if no set accessors have been used since
    the last update(). 

The docs for is_changed() have the same ambiguity.

Kingsley <kingsley@xxxxxxxxxxxxxxxx.xxx>