On Fri Oct 04 05:22:12 2013, SALVA wrote:
Show quoted text> El Jue Oct 03 12:40:26 2013, TMETRO escribió:
> > If the error handler is not used (or returns whatever the normal
> > return code is for not ignoring an error), then the method by default
> > should do the right thing and trap the error and return an error
> > indication in a way consistent with the rest of the methods in the
> > module.
>
> That makes sense, it may be useful.
> I would have to think about it.
Thanks for considering it.
Show quoted text> On the other hand, I am not sure I like the idea of dual methods that
> behave differently when "on_error" is passed or not. It doesn't seem
> obvious.
"Behave differently" is entirely up to the author of the on_error handler. More typically, an API using an error handler subroutine will look at the return code from the handler (if it does return) and use one value to signify proceeding normally with the default error handling behavior, and another value to signify short circuiting.
So if I write:
$sftp->rremove('testx/foo',
on_error => sub {
my $sftp = shift;
my $obj = shift || {};
my $file = $obj->{filename} || '<unknown>';
warn "$file: " . $sftp->error . "\n";
return 1;
}
);
my warning will get printed, and normal error handling happens. (In this case, there is no behavior change in rremove().)
While if I end that with 'return undef' it will cause the default error code in rremove() to be skipped and the method will return immediately with an error status (whatever is consistent for your API).
This sort of an API is quite common.
While that's most flexible, it's also OK to support a simpler form, where the user has to throw an exception to bail out. So to get the "normal" behavior I wanted from rremove(), I used:
$sftp->rremove('testx/foo',
on_error => sub {
my $sftp = shift;
my $obj = shift || {};
my $file = $obj->{filename} || '<unknown>';
die "$file: " . $sftp->error . "\n";
}
);
and that indeed works:
testx/foo: Couldn't stat remote link: Permission denied
I would be less concerned with the potential for on_error to alter behavior and more concerned that rremove() and other aggregate functions are inconsistent from the rest of your API in that they don't return error conditions by default.
Related to this, I noticed a few documentation bugs:
Show quoted text> $sftp->rremove(\@dirs, %opts)
[...]
Show quoted text> on_error => sub { ... }
> This callback is called when some error is occurs. The arguments
> passed are the $sftp object and the current entry (see ls docs for
> more information).
It appears ls() is not documented as accepting on_error. Did you mean find(), which appears to give the most details on on_error? (The docs for glob() also reference ls instead of find().)
You might want to flesh out the documentation for on_error in one spot, and consistently point to it from all the others (if the API is identical). (Some methods currently have a one or two sentence definition for on_error.)
The definition should also cover what is passed as the 2nd parameter (partially explained under find()), which I had to figure out from a call to Data::Dumper. The structure appears to be similar, but not identical to the structure used by ls(). (I'm not sure what value an attributes object ('a' hash key) offers in the context of a rremove() or glob() error, but I'm assuming it is there just for consistency. I see it is set to undef. If most of your methods using on_error don't need it, might be better to switch to a simple scalar.)
