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.

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

feat(samples): adds read and filter snippets #586

Merged
merged 17 commits into from Dec 3, 2019
Merged

feat(samples): adds read and filter snippets #586

merged 17 commits into from Dec 3, 2019

Conversation

billyjacobson
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 26, 2019
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #586 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #586   +/-   ##
=======================================
  Coverage   87.24%   87.24%           
=======================================
  Files          33       33           
  Lines        2046     2046           
  Branches      373      373           
=======================================
  Hits         1785     1785           
  Misses        233      233           
  Partials       28       28

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c22968...6b6d7d6. Read the comment docs.

@billyjacobson billyjacobson marked this pull request as ready for review November 26, 2019 17:26
samples/filterSnippets.js Outdated Show resolved Hide resolved
samples/filterSnippets.js Outdated Show resolved Hide resolved
samples/filterSnippets.js Outdated Show resolved Hide resolved
samples/filterSnippets.js Outdated Show resolved Hide resolved
samples/filterSnippets.js Outdated Show resolved Hide resolved
samples/test/filters.js Outdated Show resolved Hide resolved
samples/test/filters.js Outdated Show resolved Hide resolved
samples/test/filters.js Outdated Show resolved Hide resolved
samples/test/reads.js Outdated Show resolved Hide resolved
samples/writeBatch.js Outdated Show resolved Hide resolved
AVaksman and others added 3 commits November 26, 2019 20:53
* feat: add table level IAM policy controls

* lint: lint

* feat: add instance level IAM policy

* docs: update description

* docs: add document snippets

* lint: lint
Fixed headers
Removed unnceccessary asyncs
Arrow functions where possible
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Nov 27, 2019
samples/document-snippets/instance.js Outdated Show resolved Hide resolved
samples/document-snippets/instance.js Outdated Show resolved Hide resolved
samples/filterSnippets.js Outdated Show resolved Hide resolved
@billyjacobson
Copy link
Contributor Author

I'm unable to pass lint because of how I am calling the functions

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts inline 👍

samples/test/filters.js Outdated Show resolved Hide resolved
@bcoe bcoe added the cla: yes This human has signed the Contributor License Agreement. label Dec 3, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot googlebot removed the cla: no This human has *not* signed the Contributor License Agreement. label Dec 3, 2019
Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good, thanks for working with me and trying out the snapshots.

My only concern, from a stylistic point of view, is that we have been moving towards a single sample per-file (which would have addressed my eval comments). However, I understand the approach you're taking, given that there are many many small methods you're writing samples for ...

tldr; if we move to a switch statement, I'm 👍 on multiple samples in a file.

samples/readSnippets.js Outdated Show resolved Hide resolved
@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 3, 2019
@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 3, 2019
@bcoe bcoe added cla: yes This human has signed the Contributor License Agreement. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 3, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 3, 2019
@bcoe bcoe changed the title Read and Filter snippets feat(samples): adds read and filter snippets Dec 3, 2019
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 3, 2019
@bcoe bcoe added cla: yes This human has signed the Contributor License Agreement. kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 3, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 3, 2019
samples/__snapshots__/filters.js Show resolved Hide resolved
// [START bigtable_reads_row]
case 'readRow': {
const rowkey = 'phone#4c410523#20190501';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we delete the unnecessary newlines here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to separate the variable declarations from the API call, but I deleted the line between the call and print so it would be consistent across the samples

samples/readSnippets.js Outdated Show resolved Hide resolved
samples/readSnippets.js Show resolved Hide resolved
samples/package.json Outdated Show resolved Hide resolved
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 3, 2019
@bcoe bcoe 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 Dec 3, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@billyjacobson billyjacobson merged commit 896d024 into master Dec 3, 2019
@bcoe bcoe added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 3, 2019
@stephenplusplus stephenplusplus deleted the reads branch August 10, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. kokoro:force-run Add this label to force Kokoro to re-run the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants