search

ovh / node-ovh

Node.js wrapper for the OVH APIs
http://ovh.github.io/node-ovh
Other
129 stars 27 forks source link

Returns error in first parameter of callback to match node-js convention #4

Closed carolineBda closed 9 years ago

carolineBda commented 9 years ago

Following the nodejs conventions, the error should be returns as first argument of the callback.

jbblanchet commented 9 years ago

Hi,

Thank you for your contribution.

I'm not sure I see the need for this update. The callback already has a first parameter that's null if everything went well and set to the http error code if an error occurred, so the classic check if (err) { doSomething(); } works well. Whether it should be an Error object or simply the return code is debatable, but I'm not sure that it's worth a breaking change.

I'll leave this merge request open for discussion for the moment.

carolineBda commented 9 years ago

Yesterday I lost 3 hours as my call was failing and I was logging the error which was just a http status. When I finally made a real POST using curl I saw the error message and understood straight what was wrong. I think that the first parameter passed to a callback should contains all the informations about the error.

yadutaf commented 9 years ago

Hi carolineBda,

Thanks for taking the time to submit this PR. This is greatly appreciated! I do understand your frustration. As a matter of fact, following each language conventions and best practices as closely as possible is a design goal behind this wrapper.

Sadly, this PR introduces a breaking change. According to semantic versionning principles, this class of change can not be considered outside major revision. Hence I'll close this PR for now.

This said, it would be awesome if the documentation/Readme could mention this gotcha. Would you consider contributing it ?

Regards,