-
Notifications
You must be signed in to change notification settings - Fork 192
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
Cleanup WPRequest and add tests #22
Conversation
We will be using this, but not here, and not yet
(Also improves the superagent mock)
delete is a reserved word, but that won't cause any significant problems of which I am aware within the Node environment and it reserves the intent of the method to keep the verb the same as HTTP's
Ready for review: Critique welcomed, particularly on the tests! |
if ( err || result.error ) { | ||
reject( err || result.error ); | ||
} else { | ||
resolve( result ); | ||
resolve( transform( result ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmac, @carldanley and I were talking about these lines—we've both seen code like this in which the actual reject
and resolve
are return'd, rather than just called. Is there any issue of which you are aware why we'd want to return these statements, likc
if ( err || result.error ) {
return reject( err || result.error );
} else {
return resolve( transform( result ) );
}
instead of just calling the functions?
Calling
.get()
with no callback throws an error, and test coverage in WPRequest (the core of this shebang) is pretty poor. This PR rounds out the tests for the HTTP methods (excepting PATCH), and fixes some internal logical errors within WPRequest that were leading to errors or unexpected results.