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

Support toJSON() on read methods.#94

Merged
stephenplusplus merged 3 commits intogoogleapis:masterfrom
stephenplusplus:spp--27
Feb 6, 2018
Merged

Support toJSON() on read methods.#94
stephenplusplus merged 3 commits intogoogleapis:masterfrom
stephenplusplus:spp--27

Conversation

@stephenplusplus
Copy link
Copy Markdown
Contributor

@stephenplusplus stephenplusplus commented Jan 16, 2018

Fixes #27

Provide options.toJSON = true to database.run, table.read, and table.createReadStream, and all returned rows will already have had toJSON called on them.

table.read({
  keys: [...],
  columns: [...],
  toJSON: true
}, function(err, rows) {
  var row = rows[0]
  // row.Name = 'value'
})
table.createReadStream({
    keys: [...],
    columns: [...],
    toJSON: true
  })
  .on('data', row => {
    // row.Name = 'value'
  })

row.toJSON() supports options, such as wrapNumbers. To get these through, the user has to provide toJSONOption: {...} alongside toJSON: true.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 16, 2018
@ghost ghost assigned stephenplusplus Jan 16, 2018
@stephenplusplus
Copy link
Copy Markdown
Contributor Author

@WaldoJeffers want to take a look? :)

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 16, 2018

Codecov Report

Merging #94 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #94   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          11     11           
  Lines        1248   1257    +9     
=====================================
+ Hits         1248   1257    +9
Impacted Files Coverage Δ
src/partial-result-stream.js 100% <100%> (ø) ⬆️
src/database.js 100% <100%> (ø) ⬆️
src/transaction-request.js 100% <100%> (ø) ⬆️

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 718a13e...b416fe5. Read the comment docs.

rowChunks = [];

if (options.toJSON) {
formattedRows = formattedRows.map(exec('toJSON'));

This comment was marked as spam.

This comment was marked as spam.

@WaldoJeffers
Copy link
Copy Markdown
Contributor

As for passing arguments to toJSON from table.createReadStream({...}), I can't think of anything other than:

  • accepting a toJSONoptions: table.createReadStream({ toJSON: true, toJSONoptions: {...} })
  • making toJSON accept a function instead of a Boolean table.createReadStream({ toJSON: row => toJSON(row, {...}) }), but the downside is you always have to a function even if you're interested in giving it custom options

@walshie4
Copy link
Copy Markdown

Didn't dig in too deeply to see if run would end up using the modified partial-result-stream, but it looks like database.run would not support this option per the current state of this PR. Is that correct?

@stephenplusplus
Copy link
Copy Markdown
Contributor Author

stephenplusplus commented Feb 1, 2018

I've added options.toJSON and options.toJSONOptions to:

  • database.run() (thanks, @walshie4!)
  • table.createReadStream()
  • table.read()

Tests added. Please take another look!

@stephenplusplus
Copy link
Copy Markdown
Contributor Author

@alexander-fenster would you mind taking another look?

@stephenplusplus stephenplusplus merged commit d231d0e into googleapis:master Feb 6, 2018
@stephenplusplus stephenplusplus deleted the spp--27 branch February 6, 2018 20:21
@ghost ghost removed the cla: yes This human has signed the Contributor License Agreement. label Feb 6, 2018
@stephenplusplus
Copy link
Copy Markdown
Contributor Author

Since we haven't released this yet, I have a PR to rename query.toJSON to query.json and query.toJSONOptions to query.jsonOptions. Please take a look!: #122

@stephenplusplus stephenplusplus mentioned this pull request Feb 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants