Skip to content
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.

Sign up for GitHub

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

Jump to bottom

return Total Rows #431

Closed
wants to merge 7 commits into from
Closed

return Total Rows #431

wants to merge 7 commits into from

Conversation

sunnychow
Copy link

return Total Rows
#427

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Mar 6, 2015
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Mar 6, 2015
@stephenplusplus
Copy link
Contributor

Thank you for taking a crack at the issue! Can you explain what this PR is doing? I'm not sure I see how total rows is being returned, but I might be distracted by the formatting changes. Could you please keep with the style of the project (2 space indentation)?

Thanks!

@ryanseys
Copy link
Contributor

Seems the tests are failing now, so those will need to be addressed in your PR. You can run the tests locally with npm test. Regarding the approach of this PR, we don't return the raw response object for any requests so I can't see us making an exception for this case, the object that returns rows will need to be adjusted to return the totalRows property as well. This would be a backward-incompatible change which sucks, but seems necessary.

Aside: @stephenplusplus we have no clear way on how to handle these regressions caused by the addition of more attributes in the return body. We should likely discuss a best-practices approach to this because I anticipate it will happen a lot in the future and I'd prefer to not break everyone's code when it does happen and we need to change things.

@stephenplusplus stephenplusplus mentioned this pull request Mar 11, 2015
Closed
@sunnychow
Copy link
Author

I think the best practices approach to this is returns the response object. For example, in the line
https://github.com/sunnychow/gcloud-node/blob/26bca6bd7d7f909e6eeaf77cab938bf1c56a6910/lib/bigquery/index.js#L412
we should change it to

onComplete(null, rows, nextQuery, resp);

This is because google big query returns useful information in the resp. https://cloud.google.com/bigquery/docs/reference/v2/jobs/getQueryResults
For example, kind, etag, totalRows

@stephenplusplus
Copy link
Contributor

This will be solved by #448, where we return the raw API response for all calls. Thanks for getting this started, @sunnychow!

@sunnychow
Copy link
Author

thank you.

@snyk-bot snyk-bot mentioned this pull request Jan 29, 2022
Open
sofisl pushed a commit that referenced this pull request Sep 27, 2022
@xiaozhenliu-gg5
remove smoke-test hack (#431)
e9290a7
@sofisl sofisl mentioned this pull request Sep 27, 2022
Closed
4 tasks
sofisl pushed a commit that referenced this pull request Oct 12, 2022
@gcf-owl-bot
docs(samples): include metadata file, add exclusions for samples to h…
cd2e680 
…andwritten libraries (#431)

- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 429395631

Source-Link: googleapis/googleapis@84594b3

Source-Link: googleapis/googleapis-gen@ed74f97
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiZWQ3NGY5NzBmZDgyOTE0ODc0ZTZiMjdiMDQ3NjNjZmE2NmJhZmU5YiJ9
@sofisl sofisl mentioned this pull request Oct 12, 2022
Merged
4 tasks
sofisl pushed a commit that referenced this pull request Nov 9, 2022
@xiaozhenliu-gg5
remove smoke-test hack (#431)
2f3b4cf
This was referenced Nov 9, 2022
Closed
Merged
Merged
sofisl pushed a commit that referenced this pull request Nov 10, 2022
@yoshi-automation
chore: move gitattributes files to node templates (#431)
02ef511 
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/452e1583-6c83-495a-ad97-fb34fc6f653f/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@3a00b7f
@sofisl sofisl mentioned this pull request Nov 11, 2022
Merged
4 tasks
sofisl pushed a commit that referenced this pull request Nov 11, 2022
@alexander-fenster
fix(deps): require google-gax v2.12.0 (#431)
87df443
This was referenced Nov 11, 2022
Merged
Merged
sofisl pushed a commit that referenced this pull request Nov 11, 2022
@yoshi-automation
refactor(nodejs): move build cop to flakybot (#431)
cbd99c3 
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/79c017af-4fff-4b22-8cb4-5c2208642be6/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@57c23fa
sofisl pushed a commit that referenced this pull request Nov 11, 2022
@summer-ji-eng
fix: synth.py clean up for multiple version (#431)
8281580
@sofisl sofisl mentioned this pull request Nov 11, 2022
Merged
4 tasks
sofisl pushed a commit that referenced this pull request Nov 11, 2022
@yoshi-automation @JustinBeckwith
chore: release 0.12.0 (#431)
f738ba5 
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
sofisl added a commit that referenced this pull request Nov 17, 2022
@sofisl
feat!: require node 10 in engines field (#431)
ce54598
sofisl pushed a commit that referenced this pull request Nov 17, 2022
@release-please
chore: release 2.0.0 (#436)
45da634 
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.0.0](https://www.github.com/googleapis/nodejs-compute/compare/v1.2.0...v2.0.0) (2020-05-08)


### ⚠ BREAKING CHANGES

* update to latest version of gts and typescript (#433)
* require node 10 in engines field (#431)

### Features

* require node 10 in engines field ([#431](https://www.github.com/googleapis/nodejs-compute/issues/431)) ([fe2897f](https://www.github.com/googleapis/nodejs-compute/commit/fe2897fd1d625df0ccf5b910ec850e761c2ed6d3))


### Bug Fixes

* apache license URL ([#468](https://www.github.com/googleapis/nodejs-compute/issues/468)) ([#427](https://www.github.com/googleapis/nodejs-compute/issues/427)) ([02d63ed](https://www.github.com/googleapis/nodejs-compute/commit/02d63ed5a5a3559be0b5e49ed4c771b3661518a1))
* **deps:** update dependency @google-cloud/common to v3 ([#421](https://www.github.com/googleapis/nodejs-compute/issues/421)) ([d4469b6](https://www.github.com/googleapis/nodejs-compute/commit/d4469b693745df5a8a36b6988b7dba98f7d55142))
* **deps:** update dependency @google-cloud/paginator to v3 ([#419](https://www.github.com/googleapis/nodejs-compute/issues/419)) ([e667aee](https://www.github.com/googleapis/nodejs-compute/commit/e667aeecf7b9f51875c06cb6a885040828dc1181))
* **deps:** update dependency @google-cloud/projectify to v2 ([#418](https://www.github.com/googleapis/nodejs-compute/issues/418)) ([b861540](https://www.github.com/googleapis/nodejs-compute/commit/b861540aa17118a7508c7c970d84864a9028f588))
* **deps:** update dependency @google-cloud/promisify to v2 ([#417](https://www.github.com/googleapis/nodejs-compute/issues/417)) ([eedce75](https://www.github.com/googleapis/nodejs-compute/commit/eedce75e0d3f10dfed8151f12150d38d032f6a27))
* **deps:** update dependency @sendgrid/mail to v7 ([#424](https://www.github.com/googleapis/nodejs-compute/issues/424)) ([17af0f4](https://www.github.com/googleapis/nodejs-compute/commit/17af0f425354b261f9afc8ab30e16e66b7193fec))


### Build System

* update to latest version of gts and typescript ([#433](https://www.github.com/googleapis/nodejs-compute/issues/433)) ([a4fa8cc](https://www.github.com/googleapis/nodejs-compute/commit/a4fa8cc21df7a894486832b05449fa7afc02b7ee))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
This was referenced Nov 17, 2022
Merged
Merged
sofisl pushed a commit that referenced this pull request Nov 18, 2022
@fhinkel
chore(cloud-rad): delete api-extractor config (#431)
b82a473
@sofisl sofisl mentioned this pull request Jan 10, 2023
Merged
4 tasks
sofisl pushed a commit that referenced this pull request Jan 10, 2023
@chingor13
chore: remove snippet leading whitespace (#431)
4167810
sofisl pushed a commit that referenced this pull request Sep 14, 2023
@renovate-bot
chore(deps): update dependency gts to v3 (#431)
223f1c2 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [gts](https://github.com/google/gts) | devDependencies | major | [`^2.0.0` -> `^3.0.0`](https://renovatebot.com/diffs/npm/gts/2.0.2/3.0.0) |

---

### Release Notes

<details>
<summary>google/gts</summary>

### [`v3.0.0`](https://github.com/google/gts/blob/master/CHANGELOG.md#&#8203;300-httpswwwgithubcomgooglegtscomparev202v300-2020-10-08)

[Compare Source](https://github.com/google/gts/compare/v2.0.2...v3.0.0)

##### ⚠ BREAKING CHANGES

-   change default `check` to `lint` ([#&#8203;570](https://github.com/google/gts/issues/570))
-   **deps:** require TypeScript 4.x ([#&#8203;565](https://github.com/google/gts/issues/565))

##### Features

-   Add TypeScript v4 support ([#&#8203;551](https://www.github.com/google/gts/issues/551)) ([0883956](https://www.github.com/google/gts/commit/08839565a1d2b4b39d532c9b0b596f01b18856fe))
-   change default `check` to `lint` ([#&#8203;570](https://www.github.com/google/gts/issues/570)) ([c527b66](https://www.github.com/google/gts/commit/c527b66be1ef6a78ea14b3d29225a8d7fb7097bd))
-   generate .eslintignore when running init ([#&#8203;521](https://www.github.com/google/gts/issues/521)) ([8bce036](https://www.github.com/google/gts/commit/8bce0368767f0c2ad7d0700deb839962bc928d16))

##### Bug Fixes

-   add build/.eslintrc.json to files field ([#&#8203;553](https://www.github.com/google/gts/issues/553)) ([3b516ad](https://www.github.com/google/gts/commit/3b516ad5e9f0d58201dde469461db7c6ed1c1b78))
-   **deps:** require TypeScript 4.x ([#&#8203;565](https://www.github.com/google/gts/issues/565)) ([cbc5267](https://www.github.com/google/gts/commit/cbc5267579ef24e8c8ceaa2ef794df3ef54ea56a))
-   **deps:** update dependency update-notifier to v5 ([#&#8203;574](https://www.github.com/google/gts/issues/574)) ([9a882bf](https://www.github.com/google/gts/commit/9a882bf4ac30ad06e7b91a65ad5721d8e8b41c4b))
-   **deps:** update typescript-eslint monorepo to v2.34.0 ([#&#8203;509](https://www.github.com/google/gts/issues/509)) ([998a4ac](https://www.github.com/google/gts/commit/998a4ac9b75c97f04d8e5db37563f32d31652f23))
-   **deps:** update typescript-eslint monorepo to v3 (major) ([#&#8203;528](https://www.github.com/google/gts/issues/528)) ([e22e173](https://www.github.com/google/gts/commit/e22e17338db2ddb7eb829c821037c2f4e77ff869))
-   **deps:** update typescript-eslint monorepo to v4 ([#&#8203;556](https://www.github.com/google/gts/issues/556)) ([54148df](https://www.github.com/google/gts/commit/54148dfbd8b5f8b36a0f44f901c5db933393a661))
-   better error message for broken tsconfig.json ([#&#8203;501](https://www.github.com/google/gts/issues/501)) ([0c17a76](https://www.github.com/google/gts/commit/0c17a76c6650eee1d8abaff11a897a432eeaa65f))
-   prohibit calls for it.only and describe.only ([#&#8203;499](https://www.github.com/google/gts/issues/499)) ([071c33c](https://www.github.com/google/gts/commit/071c33ceef0e3765166aaebf6ed4698167ac0f98))

##### [2.0.2](https://www.github.com/google/gts/compare/v2.0.1...v2.0.2) (2020-05-11)

##### Bug Fixes

-   Revert 'update dependency eslint to v7'" ([#&#8203;507](https://www.github.com/google/gts/issues/507)) ([0f9950b](https://www.github.com/google/gts/commit/0f9950b273329dbcce5f3cc20864c3dcd076f08c))
-   **deps:** pin release of eslint-typescript ([#&#8203;508](https://www.github.com/google/gts/issues/508)) ([bd86b42](https://www.github.com/google/gts/commit/bd86b42e2bb904d3765dee82262e4691a11b9958))
-   **deps:** update dependency eslint to v7 ([#&#8203;504](https://www.github.com/google/gts/issues/504)) ([6aee159](https://www.github.com/google/gts/commit/6aee1595d0486ae2c7fd68d16b1b59c4c4015753))

##### [2.0.1](https://www.github.com/google/gts/compare/v2.0.0...v2.0.1) (2020-05-07)

##### Bug Fixes

-   throw an error if running with an unsupported version of nodejs ([#&#8203;493](https://www.github.com/google/gts/issues/493)) ([94fdf1e](https://www.github.com/google/gts/commit/94fdf1eaed634aa73c3e44c7a3d9f1325f773b07))
-   **deps:** update dependency meow to v7 ([#&#8203;502](https://www.github.com/google/gts/issues/502)) ([cf91cda](https://www.github.com/google/gts/commit/cf91cda1afab25759427511d3c97d0037d61c649))

</details>

---

### Renovate configuration

:date: **Schedule**: "after 9am and before 3pm" (UTC).

:vertical_traffic_light: **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

:recycle: **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

:no_bell: **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-dns).
@sofisl sofisl mentioned this pull request Sep 14, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sunnychow @googlebot @stephenplusplus @ryanseys