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

feat: add fieldNames option in instance#getMetadata() #760

Merged
merged 11 commits into from
Jan 16, 2020
Merged

feat: add fieldNames option in instance#getMetadata() #760

merged 11 commits into from
Jan 16, 2020

Conversation

AVaksman
Copy link
Contributor

  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Enable user to pass a list of Instance field names to be returned via getMetadata() method
proto source

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 17, 2019
@AVaksman AVaksman force-pushed the instance_getMeta_fieldMaks branch from 75f281b to ae87850 Compare December 17, 2019 05:03
@AVaksman AVaksman force-pushed the instance_getMeta_fieldMaks branch from 12f48d2 to ce3ac88 Compare December 17, 2019 05:05
@codecov
Copy link

codecov bot commented Dec 17, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #760   +/-   ##
======================================
  Coverage    74.7%   74.7%           
======================================
  Files          43      43           
  Lines       20315   20315           
  Branches      554     554           
======================================
  Hits        15177   15177           
  Misses       5133    5133           
  Partials        5       5

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 beebae6...9bbbb02. Read the comment docs.

laljikanjareeya
laljikanjareeya approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@laljikanjareeya laljikanjareeya left a comment

Choose a reason for hiding this comment

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

apart from sample and unit test LGTM!

src/instance.ts Outdated Show resolved Hide resolved
test/instance.ts Show resolved Hide resolved
@AVaksman AVaksman changed the title feat: add fieldNames option in instance#getMetadata() feat: add fieldNames option in instance#getMetadata() Dec 17, 2019
@AVaksman
Copy link
Contributor Author

While we at it, how about utilize FieldName functionality with Instance#exists() method.
Perhaps pass name into getMetadata inside the exists method as optimization effort?

@AVaksman AVaksman mentioned this pull request Dec 31, 2019
Closed
@skuruppu skuruppu requested a review from hengfengli January 8, 2020 23:57
hengfengli
hengfengli reviewed Jan 9, 2020
View reviewed changes
src/instance.ts Outdated Show resolved Hide resolved
hengfengli
hengfengli approved these changes Jan 9, 2020
View reviewed changes
Copy link
Contributor

@hengfengli hengfengli left a comment

Choose a reason for hiding this comment

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

LGTM.

@skuruppu skuruppu merged commit fa3154e into googleapis:master Jan 16, 2020
AVaksman added a commit to AVaksman/nodejs-spanner that referenced this pull request Jan 21, 2020
@AVaksman @bcoe @skuruppu
feat: add fieldNames option in instance#getMetadata() (googleapis#760)
ecfed19 
* feat: add fieldNames option in instance#getMetadata()

* docs: add a list of eligible values and a usage sample

* refactor: simplify and clean up

* refactor: use single quote

Co-authored-by: Benjamin E. Coe <[email protected]>
Co-authored-by: skuruppu <[email protected]>
@AVaksman AVaksman deleted the instance_getMeta_fieldMaks branch June 19, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hengfengli hengfengli hengfengli approved these changes

@laljikanjareeya laljikanjareeya laljikanjareeya approved these changes

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.
6 participants
@AVaksman @hengfengli @laljikanjareeya @skuruppu @googlebot @bcoe