-
Notifications
You must be signed in to change notification settings - Fork 185
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
Drop the dataset batch endpoint #112
Conversation
5baaecf
to
6b6075f
Compare
5f15140
to
d0910ad
Compare
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.
It's worth creating our own response object independent of the rest.js response. I'd propose:
return {
url: response.url,
status: response.status.code,
headers: response.headers,
entity: response.entity
}
@@ -1,3 +1,9 @@ | |||
## 1.0.0.beta4 |
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.
-beta4
owner: this.owner | ||
}, | ||
callback: callback | ||
}).entity(); | ||
}); |
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.
Curious if @tmcw has any concern about dropping .entity()
.
@scothis do you think the promise response should match the callback response object? |
Yes, the callback and promise should hold the same value. |
562fdee
to
d7041a3
Compare
@scothis - ok, I think this is ready now. |
function transform(response) { | ||
return { | ||
url: response.url, | ||
status: response.status.code, |
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.
It's possible some errors may not have a status
object.
@@ -1,3 +1,10 @@ | |||
## 1.0.0 |
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.
Are we ready for 1.0
?
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.
IDK? We've been on beta
a few times now. In an earlier commit, I called this beta4
. I can go back to that if you like.
What is our goal with 1.0?
98f34eb
to
40d2c22
Compare
As we look to move the dataset api past beta we have decided that that batch api is not something we plan to support for the long term.
This PR drops support of it fully rather than converting the batch functionality to use the single feature put and delete endpoints.
@scothis for the review