gsheets: Update input data format and allow for multiple range updates#1655
gsheets: Update input data format and allow for multiple range updates#1655hunterachieng wants to merge 12 commits into
Conversation
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
mtuchi
left a comment
There was a problem hiding this comment.
I have asked joe, for his opinion on new signature
batchUpdateValues(sheetId, data = [{range, values}], options = {valueInputOption})
If he agrees i think, we should update appendValues as well and remove support for callback in getValues()
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
mtuchi
left a comment
There was a problem hiding this comment.
@hunterachieng i think the changes are looking good, can you tidy up two things
- Create an issue supporting array of data in
appendValues(id, data = [{}], options)to match what we have inbatchUpdateValues - Clean up PR description and title to match the changes and don't forget to update the review checklist as well
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
Signed-off-by: Hunter Achieng <achienghunter@gmail.com>
|
Hey @hunterachieng sorry for long review, i wanted to confirm few things on The current implementation does not multiple range when using See writing multiple range docs. I honestly don't think we need to support multiple range in |
|
These examples from google are really useful, It's was really hard to find a good documentation on how to use the googleapis sheets api in node |
mtuchi
left a comment
There was a problem hiding this comment.
@hunterachieng I have update pr notes and remove this issue from the notes #1656, See my comments here.
I am happy to handover this issue to joe for final round of review
|
Re appendValues: yes, appendValues should take a single range/value set, and bulk should take multiple. As the original issue hints at, I don't understand the difference really between |
| * values: [ | ||
| * ['From expression', '$15', '2', '3/15/2016'], | ||
| * ['Really now!', '$100', '1', '3/20/2016'], | ||
| * <caption>Update a single range</caption> |
There was a problem hiding this comment.
It would be nice for the single range API to take a single object, rather than an array of one. Just a little API convenience
There was a problem hiding this comment.
@josephjclark can you say more 🤔 , values are rows data do you mean we should pass values: [{columnHeader: value, colHeader: value}, {...}]?
There was a problem hiding this comment.
So multiple ranges is:
batchUpdateValues(id, [set1, set2])
Where a set is { range, values }
And a single range so far is:
batchUpdateValues(id, [set1])
I'm saying allow a single range to be passed like this:
batchUpdateValues(id, set1)
BUT THEN AGAIN
That makes the API the same as the appendValues API, which only confuses the two.
Ok, so we should force data to be an array of ranges. But do not show an example with an array of just 1 range because it's wierd
There was a problem hiding this comment.
I think function naming is what causing the confusion here. There are singular actions and plural actions
For example:
- If we had
appendValue(id, range, data, opt)this is clear, add new row to gsheet - For
appendValues(id, [{set0, set1}], opt), this is clear adding multiple rows
The same could apply for batchUpdateValues()
batchUpdate(id, range, value,opt)batchUpdate(id, [{set0}, {set1}])
Of course adding good docs that explain what append does, and what does batchUpdate do? because their different.
I will remove the single batch update example
|
…s args Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Updated
appendValues(),batchUpdateValues(), andgetValues()to use positional arguments instead of a single params object.Callback parameter has been removed from
appendValues(),batchUpdateValues(), andgetValues()in favor of a promise-based API.Fixes #1476
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
production? Is it safe to release?
dev only changes don't need a changeset.