[go: nahoru, domu]
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

Overload getAll function to allow array destructuring #515

Merged
merged 8 commits into from
Jan 9, 2019
Merged

Overload getAll function to allow array destructuring #515

merged 8 commits into from
Jan 9, 2019

Conversation

cxam
Copy link
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
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!

dev/src/index.ts Outdated Show resolved Hide resolved
@cxam
Copy link
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
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.

dev/src/index.ts Outdated Show resolved Hide resolved
dev/src/index.ts Outdated Show resolved Hide resolved
dev/system-test/firestore.ts Show resolved Hide resolved
@cxam
Copy link
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
@cxam
Copy link
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
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
Contributor

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

@cxam
Copy link
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
Member
jkwlui commented Jan 9, 2019

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

@mikelehen
Copy link
Contributor

@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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@mikelehen mikelehen mikelehen approved these changes

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.

None yet
6 participants
@cxam @mikelehen @jkwlui @schmidt-sebastian @googlebot @kokoro-team