Return value

[djzort]

I just wanted to discuss one thing that kind of bothers me with this base client library, which i have otherwise found extremely useful.

That is inconsistent behavior of return values. Please dont misunderstand that comment, as things are behaving as they are documented.

However, depending on the HTTP code the return value will either be a json string, undef or i will need to catch an exception. In the first two cases, i dont get to inspect the entire reply (easily) and in the third case i case i have to wrap everything in eval {} or try {} then look for $@ etc.

try {
   my $result = $obj->foo($etc);
}
catch {
  # do something if i get certain http codes, which may not be "errors"
};
unless ($result) {
  # handle no result being returned
}
if ($result....) {
   # actually do something with the result
}

For that reason, i would suggest instead that a WebService::Client::Response object be defined which is always returned regardless of the HTTP code, which would also have the http response headers, the json decode and the original response.

in which case you would always do something like:

my %dispatch =(
 404 => sub {},
 200 => sub {},
);

my $result = $obj->foo();

$dispatch{ $result->code }->();

Or a if/elseif/elseif/else chain, or a switch statement...

The return value may be an "error" in terms of http. but in a rest context its a meaningful return value. So having to catch it as a special case is inconsistent.

What do you think?

[ironcamel]

Hi, and thanks for your thoughtful suggestion. I like the idea of always returning a WebService::Client::Response object. It would make the behavior more consistent for users of the library, and it would also remove complexity in the library itself. But how do you suggest making this change in a backwards compatible way? I would prefer to not break existing clients that use this library.

[djzort]

I think you would have to add complexity by having a variable or something that indicated the desired return response. The various methods would then have to respond to it according.

Perhaps like this:

extends 'WebService::Client';
$WebService::Client::RespondWithObject = 0;

Or as a parameter for each object

has 'respond_with_object' => {
is => 'ro',
default => 0
};

Pick names and defaults that are well considered, these are just quickly typed as examples

[djzort]

My other thought on this is to provide a base class using Throwable, that people can extend which would be used for exceptions. Im currently achieving this using the Moo 'around' for every verb

[ironcamel]

I'm not exactly sure what you mean. Can you share example code?

[djzort]

In the main package:

{

my $ss = sub {
my $orig = shift;
my $self = shift;
my $result;
eval { $result = $orig->($self,@_) };
if ($@) {
Net::KairosDB::REST::Error->throw({
code => $@->code,
message => $@->message
})
}
return $result
}; # my $ss

around 'get' => $ss;
around 'post' => $ss;
around 'delete' => $ss;

}

[djzort]

package Net::KairosDB::REST::Error;

use Moo;
with 'Throwable';
use namespace::clean;

has code => (is => 'ro');
has message => (is => 'ro');

1;

[djzort]

Thats not ready for the CPAN yet, but hopefully you can follow it

[djzort]

So when get, post, or delete would die and send a HTTP::Response object, instead its converted in to a ::Error object with a simplified interface then rethrown. Because its Throwable, $@ is automagically stored in the new ::Error object too

[ironcamel]

Hey @djzort, here is my first stab of implementing your idea of adding a mode where a response object is always returned: #19

What do you think?

Sorry for the delay by the way. I finally got some free time and am looking forward to improving this module. Thanks for your help and suggestions!

[djzort]

The approach seems perfectly reasonable. I would suggest that it be controllable with both a 'use' option and an option in new()... this makes it a one line change globally or case by case

[ironcamel]

Can you point me to an example of another module that has a 'use' option?