-
Notifications
You must be signed in to change notification settings - Fork 10
Support Promises #5
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
Conversation
- updated package.json and readme to reflect that
- Test fails
- added returns to all resources for this to work
Huh, the tests pass, but the command to prettify it for travis is failing for some reason. I'll see if I can get that to work. |
- Follows style of other code - Plus it was making the parser upset for the tests
|
||
//request doesn't think 4xx is an error - we want an error for any non-2xx status codes | ||
//we also we want this to be a real error object... | ||
if (!err && (response.statusCode < 200 || response.statusCode > 206)){ |
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.
Why not check for > 299
rather than > 206
to cover all 2xx?
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.
That's from the original function, so that one's on me, not @cgpuglie. It's been a while, but if I recall correctly this was something I saw in another SDK that took a similar approach to non-success code handling when using Request. I personally don't see any issue with just making the logic > 299
, but to be fair 206 is the last "official" 2xx status code. ¯\_(ツ)_/¯
.reply(200, scopes.accounts.details) | ||
.post('/api/v90/jobs') | ||
.post('/api/v90/jobs').times(2) |
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.
Why /api/v2/
and then /api/v90
?
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.
There's a test to ensure the 404 error handling returns an error object.
Looks good, just had 2 miscellaneous questions. |
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.
🔥🔥🔥
Not that my approval matters of course, just checked out the PR when I was updating the npm maintainers :)
|
||
//request doesn't think 4xx is an error - we want an error for any non-2xx status codes | ||
//we also we want this to be a real error object... | ||
if (!err && (response.statusCode < 200 || response.statusCode > 206)){ |
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.
That's from the original function, so that one's on me, not @cgpuglie. It's been a while, but if I recall correctly this was something I saw in another SDK that took a similar approach to non-success code handling when using Request. I personally don't see any issue with just making the logic > 299
, but to be fair 206 is the last "official" 2xx status code. ¯\_(ツ)_/¯
.reply(200, scopes.accounts.details) | ||
.post('/api/v90/jobs') | ||
.post('/api/v90/jobs').times(2) |
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.
There's a test to ensure the 404 error handling returns an error object.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "zencoder", | |||
"version": "1.1.0", | |||
"version": "2.0.1", |
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.
For what it's worth, this feels like it could technically be a minor bump if you wanted to. As far as I can tell no current functionality is broken (👍 ), so it shouldn't break anything for folks that are pinned to 1.x
.
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 went with major since it will no longer work with node 0.1.0, which was the original version requirement. Wanted to use native promises which require 6.4 or above.
I'm open to doing a minor if you'd prefer it though.
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.
That reasoning makes total sense to me 👍
Resolve #3
Added promise support and associated tests and documentation.