-
Notifications
You must be signed in to change notification settings - Fork 305
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
Workbook package #3019
base: version-4
Are you sure you want to change the base?
Workbook package #3019
Conversation
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.
Really nice work! You did a great job figuring out and integrating with the library. I left a few comments for you to consider. Excited for your contribution!!
1. Your contributing guidelines mention I should skip writing tests for endpoints that don't support app auth. According to the docs, that's all of the workbook endpoints. Is there any way I can write tests we can use for these endpoints? Not really. You can test them locally. We are working on test recording to allow us to record delegated-auth tests and replay them to validate. 2. In my testing, I wasn't able to use workbook operations on items opened by path. These failed with "Could not obtain a WAC token". When I use getItemById, it works. I wrote this off as a Graph API bug, but haven't tested extensively. Don't know, WAC is the office protocol, perhaps that endpoint requires something special. 3. My current implementation of workbook sessions involves a I am not deeply familiar with this API, what is the difference between sessionless and with session? Do we need to expose both? 4. I've added a prefer-async behavior to the graph folder. This is to handle the long-running operation pattern that a few of the workbook endpoints support. Again, I'm not sure if this is the ideal approach, but it's what I've got at the moment. This is pretty slick, I left a few notes inline. This isn't something we built-in for v4 launch, but I can see a ton of value to including it. Again, nice work! |
A few of the collections from the workbook endpoints support getItemAt(index). I've implemented this in the same pattern as the getById decorator. Currently I've only added it to the rows collection.
- Replaced manual setTimeout with delay() - Directly invoked statusQuery rather than calling graphGet - Wrapped handler with standard library error checking via parseBinderWithErrorCheck
Oops! I didn't read parseBinderWithErrorCheck properly and missed that it runs the impl function on all HTTP successes. I only want prefer-async to trigger on 202 Accepted. Updated my behaviour to manually add an errorCheck parse handler before my own.
HI @ChapC is this ready? We see your changes, we were just waiting on anymore feedback from you. |
Endpoints for getting the Range contained by tables, rows, columns and worksheets. Plus added the 'clearFilter' endpoint to Table.
Hey @bcameron1231, I've just added a few more of the endpoints I've tested recently. I was hoping to cover much more of the workbook functionality, but I've ended up with much less time than I thought I would have on this. Apologies! So far, of all the workbook sections, I've implemented the basic stuff in Tables, Worksheets, Rows/Columns and Ranges. Everything to get/update/delete data in the spreadsheet. I have mocha tests for most of these on my local machine, although they're in a separate project as I had trouble getting it working with this one. There were a couple quirks I encountered in these endpoints. Most I mentioned previously, but the biggest one I couldn't find a fix for was incompatibility with paging. The async iterator doesn't work for table rows, because as far as I can tell the Graph API simply doesn't return There's still quite a bit more for full coverage of all endpoints, like charts, running formulas and cell styling. I'm happy to keep chipping away at these features, just won't have a timeframe for you unfortunately. I can get some documentation written up soon if you guys decide you'd like to merge just the stuff I've got working rather than the full set. Just let me know :) |
Also, to answer the question from @patrick-rodgers regarding session vs sessionless; my understanding is that both work for pretty much every endpoint, but using a session leads to better performance. It's briefly explained in the docs here. To quote them directly:
It also allows you to create non-persistent sessions, in which you can edit the workbook and run calculations without your changes being saved to the file. In sessionless mode, your changes are always immediately saved to the file. |
Sounds good @ChapC! No worries, we're all busy so appreciate any time you have to contribute 😀 Let us know once you're ready for a full review. Thank you!! |
Added endpoints for Range formatting. That includes borders, fill, font, row/column sizing, text alignment and protection.
Added a few more endpoints (sorting, various range getters) to fully cover table manipulation.
Category
Related Issues
Mentioned in #3016
What's in this Pull Request?
Initial draft of Excel workbook support.
This first commit includes
I'll implement more endpoints as I go, but have a couple of notes/questions I thought I'd post here first.
workbook
property on DriveItem that leads to a regular, sessionless IWorkbook and an async methodgetWorkbookSession
that resolves to an IWorkbookWithSession. The latter is an extension of IWorkbook that adds session refresh/close methods, and uses the InjectHeaders behaviour to add the session id header. Thoughts on this approach?Welcoming all feedback. Thanks!