Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

Overload getAll function to allow array destructuring#515

Merged
mikelehen merged 8 commits intogoogleapis:masterfrom
cxam:fix-getall-destructuring
Jan 9, 2019
Merged

Overload getAll function to allow array destructuring#515
mikelehen merged 8 commits intogoogleapis:masterfrom
cxam:fix-getall-destructuring

Conversation

@cxam
Copy link
Copy Markdown
Contributor

@cxam cxam commented Jan 8, 2019 •
edited
Loading

Fixes #501

This PR overloads the getAll function in Firestore and Transaction class to allow passing in a destructured array of documents.

  • Tests and linter pass
  • Code coverage does not decrease
  • Appropriate docs were updated

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 8, 2019
@cxam cxam mentioned this pull request Jan 8, 2019
Closed
mikelehen
mikelehen suggested changes Jan 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Thanks very much for digging in and taking a stab at overloads (and adding tests, etc.!) but looking closer I think overloads don't actually help here. So do you mind going dropping the "overload" and just using your more permissive function signature like you did in the original PR? Thanks!

Comment thread dev/src/index.ts Outdated
@cxam
Copy link
Copy Markdown
Contributor Author

cxam commented Jan 8, 2019 •
edited
Loading

@mikelehen I have just pushed an update to the PR that uses a tuple type along with the rest element. What this means is that the first parameter is required and must be of type DocumentReference followed by the rest. This is just an improvement over the original signature to prevent calling getAll() without any matching parameters or an incorrect one (getAll(ReadOptions)).

mikelehen
mikelehen suggested changes Jan 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Thanks! Left a couple more comments. Sorry for so much back-forth.

Comment thread dev/src/index.ts Outdated
Comment thread dev/src/index.ts Outdated
Comment thread dev/system-test/firestore.ts
@cxam
Copy link
Copy Markdown
Contributor Author

cxam commented Jan 8, 2019

No worries! Just updated the PR with single signature. This should hopefully maintain compatibility albeit being overly permissive.

@jkwlui jkwlui added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2019
@jkwlui jkwlui assigned jkwlui and cxam and unassigned jkwlui Jan 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2019
schmidt-sebastian
schmidt-sebastian reviewed Jan 9, 2019
View reviewed changes
Comment thread dev/src/index.ts
@cxam
Copy link
Copy Markdown
Contributor Author

cxam commented Jan 9, 2019

@schmidt-sebastian JSDoc updated in latest commit. I have also fixed the failing test for Transaction class - getAll() supports array destructuring with field mask.

Also, I noticed that npm test doesn't actually run the system tests. I understand that this is due to the credentials needed to run this properly as it requires access to a Google Cloud Project. Should the contributing guide be updated to include instructions on how to run these (i.e. setting of GCLOUD_PROJECT and GOOGLE_APPLICATION_CREDENTIALS environment variables)?

mikelehen
mikelehen approved these changes Jan 9, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Thanks! I think this looks good to me now. 😁

@mikelehen
Copy link
Copy Markdown
Contributor

mikelehen commented Jan 9, 2019

@JustinBeckwith @crwilcox Either of you know if there's a trick to making the CI run (for an externally contributed PR)?

@cxam
Copy link
Copy Markdown
Contributor Author

cxam commented Jan 9, 2019 •
edited
Loading

@mikelehen I think last time this worked when @kinwa91 assigned the kokoro:force-run label (googleapis/sloth).

@jkwlui jkwlui added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 9, 2019
@jkwlui
Copy link
Copy Markdown
Contributor

jkwlui commented Jan 9, 2019

Anyone with write access can apply the kokoro:force-run label to jumpstart the test.

@mikelehen
Copy link
Copy Markdown
Contributor

mikelehen commented Jan 9, 2019

@cxam @kinwa91 Nice, thanks!

@mikelehen mikelehen merged commit 87024b3 into googleapis:master Jan 9, 2019
@mikelehen mikelehen mentioned this pull request Jan 10, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Dec 5, 2019
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@schmidt-sebastian schmidt-sebastian schmidt-sebastian left review comments

@mikelehen mikelehen mikelehen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cxam cxam

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.

6 participants

@cxam @mikelehen @jkwlui @schmidt-sebastian @googlebot @kokoro-team