Return true/false result for destroyById/prototype.destroy

[fabien]

See also: #321 - note that the memory connector has been fixed to have destroy also return { count: 1 } feedback - this should also be done with the other connectors.

/cc @raymondfeng @bajtos @superkhau

[bajtos]

Can we come up with a better name please? count === 0 is not a failure, it just mean that the record has been already deleted by somebody else before us.

[bajtos]

Perhaps options.failWhenNotFound, options.mustExist or options.strict.

Another approach is to use a property with true as a default value, e.g. options.ignoreMissing, options.lenient.

[bajtos]

Also note that !deleted will be triggered also in the case when the connector does not report back the number of deleted records. You should use deleted === false instead.

[bajtos]

The implementation looks good in general, I left a bunch of comments regarding details that I'd like to improve. @raymondfeng PTAL too.

[fabien]

@bajtos the flag I propose: strict

[bajtos]

Please set an appropriate http status code via err.statusCode, e.g. "404 Not Found". This needs to be fixed in other places too.

[raymondfeng]

Naming wise, I'm fine with with strict (default to false) or force (default to true).

[bajtos]

@fabien the flag I propose: strict
@raymondfeng Naming wise, I'm fine with with strict (default to false) or force (default to true).

Let's go with strict (default to false) then.

[fabien]

@bajtos let me know if you want those commits squashed

[raymondfeng]

Yes, please squash the commits.

BTW, do the tests have dependencies on connector implementations? Some of the tests from this module are also run with connectors.

[bajtos]

Things to address:

• the error objects should have a reasonable HTTP statusCode property and ideally also a machine-readable code. E.g. statusCode: 404, code: 'NOT_FOUND'
• the callback argument should be an object ({ deleted: false }) for future extensibility as discussed in the comments above

I am not happy about the should.not.exist(err); pattern, but I am not going to repeat myself.

[bajtos]

Note that some of the PRs that @superkhau is working on are changing the callback arguments from err, data to err, data, info. This patch may need to reflect that.

[raymondfeng]

Now we settle the callback for updateAll and deleteAll to be cb(err, info).

[raymondfeng]

Let's do err.statusCode = 404.

[raymondfeng]

@fabien ping.

[fabien]

@raymondfeng I'll see if I can work on this later.

[bajtos]

@fabien I'll see if I can work on this later.

Ping, what's the status of this work? If you won't have time to work on this in the next few weeks, then I am proposing to close this pull request for now. You can always reopen it when you get more time.

[bajtos]

Ping, what's the status of this work? If you won't have time to work on this in the next few weeks, then I am proposing to close this pull request for now. You can always reopen it when you get more time.

@fabien ping again. Would you mind if I close this pull request, since it seems to be abandoned?

/cc @raymondfeng

[bajtos]

Closing in favour of #679.