-
Notifications
You must be signed in to change notification settings - Fork 2.3k
simplifies slack web api instantiation #319
Conversation
|
||
// generate all API methods | ||
slackApiMethods.forEach(function(slackMethod) { | ||
// most slack api methods are only two parts, of the form group.method, e.g. auth.test |
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.
All comments should be written using JSDoc. Can you update all of these comments, please?
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.
Sure thing, is there a jsdoc convention for inline docs like this? Most of what I know is for method signatures or class definitions.
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.
I think I got all the JSDoc ironed out 😄
* @param options | ||
* @param cb | ||
*/ | ||
function postForm(url, options, cb) { |
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.
I also pulled out this request.post and any other duplicate code, this reduces callAPI and callAPIWithoutToken to constructing options objects, all other request logic is handled here
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.
Having a function and removing the code duplicate parts is a good choice!
8d487e3
to
dc56ba6
Compare
try { | ||
json = JSON.parse(body); | ||
} catch (parseError) { | ||
return cb(parseError); |
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.
You've indicated that callback is optional. But you didn't check if cb is defined or not. Is it truely optional?
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.
The first line checks for a callback and if it's not defined sets it to the noop function. That simplifies the checks later since cb will always be defined.
The checks on cb were there before I refactored so it seems the previous behavior was to allow optional callbacks. I'm keeping with that for backwards compatibility.
@colestrode sorry it took so long to merge this! |
No problem @benbrown glad to help 😀 |
This change simplifies the creation of the Slack web api interface and reduces the amount of copy paste code.
I also simplified the callbacks in the callAPI and callAPIWithoutToken methods.