-
-
Notifications
You must be signed in to change notification settings - Fork 391
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.
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
Implement Sort range #379
base: main
Are you sure you want to change the base?
Implement Sort range #379
Conversation
We were running into rate limits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this :D
Couple things - curious what you think:
- One thing that may be a little confusing is if the user is using numbers as header row values? Would you just pass in a stringified number? anything else we could do about that?
- maybe making the function accept a keyed object instead of array of arrays could be cleaner (or better yet, accept both). For example
sheet.sortRange(somerange, { header1: 'DESC', header2: 'ASC' })
- maybe make the range optional, so if you just pass sort settings, it sorts the whole sheet? I imagine thats probably the most common scenario. If we do this, might be easier to make the range the second argument.
and yes if you can, add docs! it's pretty easy - see https://raw.githubusercontent.com/theoephraim/node-google-spreadsheet/master/docs/classes/google-spreadsheet-worksheet.md
// [[rowIndex, order], [rowIndex, order]...] where order is 'desc' or 'asc' | ||
// https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets/request#SortRangeRequest | ||
async sortRange(sortSpecs, range) { | ||
if (!sortSpecs.length) return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better to throw an error?
}; | ||
|
||
sortSpecs = sortSpecs.map(([idx, dir]) => ({ | ||
sortOrder: dirmap[dir], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe good to use toUpperCase
or toLowerCase
on what gets passed in to support 'desc' and 'DESC' etc
// https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets/request#SortRangeRequest | ||
async sortRange(sortSpecs, range) { | ||
if (!sortSpecs.length) return null; | ||
if (!_.isArray(sortSpecs[0])) { sortSpecs = [sortSpecs]; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter will probably catch it but dont need {}
around a one liner
|
||
sortSpecs = sortSpecs.map(([idx, dir]) => ({ | ||
sortOrder: dirmap[dir], | ||
dimensionIndex: typeof idx === 'number' ? idx : this.headerValues.indexOf(idx), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if the header row doesnt exits? Maybe should throw an error?
Hi, I'm trying to implement a sort entire sheet by column function, @cincodenada did you make any further progress on this? |
@cincodenada @theoephraim please ignore previous comment, I have solved it haha |
cf803e0
to
53fe384
Compare
Implements the sortRange() method.
I implemented a shorthand for the sortSpecs, but that can be changed if needed.
Also, I should probably add docs about this somewhere.