Fixed return value from delete command.

[ngnpope]

The delete() command currently returns 1 if the response from the server is either 'DELETED' or 'NOT_FOUND'. This differs from other implementations, e.g. pymemcache and pylibmc, and doesn't seem to make sense given that these are the only two documented return types from the protocol for the delete command.

It seems that delete() was changed to explicitly consider both responses as successful way back in 2010, but that change only seemed to double down on the behavior that was being reported in the issue. See https://bugs.launchpad.net/python-memcached/+bug/471727. The concern was avoiding breaking backward compatibility.

It was possible to work around this by calling _deletetouch() and passing in the expected responses, but that was broken by the change to remove time in ab668ed.

To avoid issues with backward compatibility, I've added a strict argument which can be enabled to return a non-successful response from delete() for 'NOT_FOUND'.

Further, making the change to interpret 'NOT_FOUND' as 0 doesn't cause any test failures. I've added an assertion to check deletion of a non-existent key using both strict and non-strict.

Fixes #170.

[ngnpope]

To avoid issues with backward compatibility, I've added a strict argument which can be enabled to return a non-successful response from delete() for 'NOT_FOUND'.

This would be much cleaner avoiding the flag and dropping 'NOT_FOUND' if you think it would be acceptable 🤷🏻

[linsomniac]

I am open to the idea that "NOT_FOUND" is a failure, as no deletion took place. I'd go so far as to say I think the current behavior is wrong, and question the sanity of the maintainer back in 2010, but I imagine the thinking was along the lines that it's safer to return success than possibly have code break because it got a "0" and was thinking there was a memcache problem.

Strictly from reading the docstring: "Deletes a key from the memcache" and "Returns nonzero on success", I think NOT_FOUND is not success. It also isn't a failure.

The issue is that delete() also returns 0 when the server is down. So having NOT_FOUND return "0" means that you can't tell the difference between a negative (key is not in server) and an unknown (I wasn't able to check for the key in the server). But I'd be willing to call that an issue in the API contract ("return non-zero on success").

Without adding a new API endpoint with a different contract (say, returning deleted/not_found/failed), I think this may be our best bet: honoring the contract.

I'm struggling to find a place where NOTFOUND returns 0 would be a breaking change.

Opening for discussion.

[ngnpope]

Yes. It's a little bit messy.

I'm struggling to find a place where NOTFOUND returns 0 would be a breaking change.

I think most people are probably not checking the return value of delete() anyway - they want something deleted and it either gets deleted, wasn't found anyway, or the service wasn't there (which effectively means it's deleted anyway).

Also, the service going away typically will have wider issues than whether this particular command succeeded.

It feels more useful to know where a key you expected to be present was successfully deleted or not there as you expected it to be.

I'll update this PR to treat 'NOT_FOUND' as unsuccessful to honour the documented contract and be consistent with other implementations.

[ngnpope]

I've now updated this as discussed.

[linsomniac]

LGTM, thanks for working on this!