Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

carolineBda
Copy link

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

@jbblanchet
Copy link

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
Copy link
Author

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
Copy link
Contributor

yadutaf commented Aug 7, 2015

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,

@yadutaf yadutaf closed this Aug 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants