Re: Discrepancies in docs and implementation

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

FWIW, I agree with your comments. Thanks for spotting these issues.

Tim.

On Fri, Oct 08, 2004 at 04:45:20PM -0400, Kingsley Kerce wrote:
> 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>
>