Added method to obtain most recent result message (NOT_FOUND etc).

I have attached a patch which allows obtaining the most recent protocol
result (eg. NOT_FOUND etc) for non-get commands.

This is useful if you eg. want to distinguish between NOT_FOUND and some
other error (eg. lost connection, protocol error etc.), or different set fails (eg. size too big etc.)

Patch is created against svn trunk

[brad_danga_com, 2009-02-28 15:58:40]

http://codereview.appspot.com/22052/diff/1/2
File lib/Cache/Memcached.pm (right):

http://codereview.appspot.com/22052/diff/1/2#newcode90
Line 90: sub last_res {
"res" is a great internal abbreviations but it's a terrible symbol to let leak
out in a public API.  This should be renamed to "last_result" probably.

http://codereview.appspot.com/22052/diff/1/2#newcode414
Line 414: $self->{last_res} =~ s/\r\n$//;
Performance suggestion:  It might make more sense to only do the \r\n stripping
lazily, when the user actually calls the "last_result" method, so you avoid the
regexp in the common case (of the user not calling the method).

http://codereview.appspot.com/22052/diff/1/2#newcode1112
Line 1112: This method returns the status string of the last operation except
for get(s).
So this only works with the ASCII protocol?  You should be explicit about that. 
Perhaps document that if this module were ever to support the binary protocol,
this method would continue to return the ASCII *equivalent* status code, even if
the library decided to speak binary behind the scenes.

[markaufflick, 2009-03-02 00:04:12]

reply comments - patch to follow

http://codereview.appspot.com/22052/diff/1/2
File lib/Cache/Memcached.pm (right):

http://codereview.appspot.com/22052/diff/1/2#newcode90
Line 90: sub last_res {
Fair point - i'll change that.

On 2009/02/28 15:58:41, bradfitz wrote:
> "res" is a great internal abbreviations but it's a terrible symbol to let leak
> out in a public API.  This should be renamed to "last_result" probably.

http://codereview.appspot.com/22052/diff/1/2#newcode414
Line 414: $self->{last_res} =~ s/\r\n$//;
Also a good suggestion.

On 2009/02/28 15:58:41, bradfitz wrote:
> Performance suggestion:  It might make more sense to only do the \r\n
stripping
> lazily, when the user actually calls the "last_result" method, so you avoid
the
> regexp in the common case (of the user not calling the method).

http://codereview.appspot.com/22052/diff/1/2#newcode1112
Line 1112: This method returns the status string of the last operation except
for get(s).
The whole module has no support or mention of the binary protocol, but I would
envision binary support to work the way you mention. No point having a pile of
constants that have to be exported/imported when we can use the simple strings
of the ascii protocol.

I'll include some sort of comment to that effect.

On 2009/02/28 15:58:41, bradfitz wrote:
> So this only works with the ASCII protocol?  You should be explicit about
that. 
> Perhaps document that if this module were ever to support the binary protocol,
> this method would continue to return the ASCII *equivalent* status code, even
if
> the library decided to speak binary behind the scenes.

[markaufflick, 2009-03-02 00:16:08]

updated patch as per code review