-
Notifications
You must be signed in to change notification settings - Fork 642
Add apiResponses #448
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
Add apiResponses #448
Conversation
|
Looking clean so far 👍 |
|
Thanks! Do you think this is appropriate for datastore or what approach should we take there? |
|
We do a lot of convenience wrapping of the response (all that |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
TADA! 🎆 🚨 This PR should be ready for a review. This PR goes down in my books as one of the most tedious and boring PRs of all time. 😄 |
|
I plan to squash, but I figure it might be easier to review with the nice separate diffs. |
|
I left out |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@sindresorhus We've been designing gcloud-node to give users curated, massaged responses from their API calls. The upstream API response can be quite intimidating/non-user friendly, which is why we try to clean it up. However, we've hit a couple places where we forgot to include something and/or didn't want to include it, because it doesn't fit the majority use case. To support cases like these, we want to give users the raw API response in a "it's there if you need it, ignore it if you don't care" kind of way. This PR is our solution, which tacks on that raw response as the final argument to their callback: // old
bucket.upload('file.jpg', function (err, file) {});
// new
bucket.upload('file.jpg', function (err, file, apiResponse) {});Quite a bit uglier, but we have methods that return more than just two arguments to the callback: // old
dataset.get(key, function (err, results, nextQuery) {});
// new
dataset.get(key, function (err, results, nextQuery, apiResults) {});I'm wondering if you've come across APIs that have solved for the same problem, and if our solution seems reasonable. Thanks! |
|
Seems this has gotten out of sync with master. I'll rebase once we hear back from sindre. |
|
Cool, thanks! Once the out of sync-ness is resolved, I think we're merge-ready. |
|
Rebased and squashed. Merge when ready! |
|
Yay! |
|
Wewt! |
|
thanks! |
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [sinon](https://sinonjs.org/) ([source](https://togithub.com/sinonjs/sinon)) | [`^10.0.0` -> `^11.0.0`](https://renovatebot.com/diffs/npm/sinon/10.0.0/11.1.0) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>sinonjs/sinon</summary> ### [`v11.1.0`](https://togithub.com/sinonjs/sinon/blob/master/CHANGELOG.md#​1110--2021-05-25) [Compare Source](https://togithub.com/sinonjs/sinon/compare/v11.0.0...31be9a5d5a4762ef01cb195f29024616dfee9ce8) \================== - Add sinon.promise() implementation ([#​2369](https://togithub.com/sinonjs/sinon/issues/2369)) - Set wrappedMethod on getters/setters ([#​2378](https://togithub.com/sinonjs/sinon/issues/2378)) - \[Docs] Update fake-server usage & descriptions ([#​2365](https://togithub.com/sinonjs/sinon/issues/2365)) - Fake docs improvement ([#​2360](https://togithub.com/sinonjs/sinon/issues/2360)) - Update nise to 5.1.0 (fixed [#​2318](https://togithub.com/sinonjs/sinon/issues/2318)) ### [`v11.0.0`](https://togithub.com/sinonjs/sinon/blob/master/CHANGELOG.md#​1100--2021-05-24) [Compare Source](https://togithub.com/sinonjs/sinon/compare/v10.0.1...v11.0.0) \================== - Explicitly use samsam 6.0.2 with fix for [#​2345](https://togithub.com/sinonjs/sinon/issues/2345) - Update most packages ([#​2371](https://togithub.com/sinonjs/sinon/issues/2371)) - Update compatibility docs ([#​2366](https://togithub.com/sinonjs/sinon/issues/2366)) - Update packages (includes breaking fake-timers change, see [#​2352](https://togithub.com/sinonjs/sinon/issues/2352)) - Warn of potential memory leaks ([#​2357](https://togithub.com/sinonjs/sinon/issues/2357)) - Fix clock test errors ### [`v10.0.1`](https://togithub.com/sinonjs/sinon/blob/master/CHANGELOG.md#​1001--2021-04-08) [Compare Source](https://togithub.com/sinonjs/sinon/compare/v10.0.0...v10.0.1) \================== - Upgrade sinon components (bumps y18n to 4.0.1) - Bump y18n from 4.0.0 to 4.0.1 </details> --- ### Configuration 📅 **Schedule**: "after 9am and before 3pm" (UTC). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻️ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/nodejs-os-login).
* Add semistandard as a devDependency * Fix storage tests * Fix language tests * Update (some) dependencies * Fix language slackbot tests
* Add semistandard as a devDependency * Fix storage tests * Fix language tests * Update (some) dependencies * Fix language slackbot tests
* Add semistandard as a devDependency * Fix storage tests * Fix language tests * Update (some) dependencies * Fix language slackbot tests
* Add semistandard as a devDependency * Fix storage tests * Fix language tests * Update (some) dependencies * Fix language slackbot tests
Adding
apiResponseto the callbacks of every API.Regression tests(doesn't make sense)Fixes #434