-
Notifications
You must be signed in to change notification settings - Fork 566
Fix next page token #70
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
Conversation
|
@tdas can you review this PR? |
433cabf to
02c88af
Compare
ramesh-db
left a comment
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.
@abrassel thank you for fixing this.
Can you add a test for the fix? Will merge after the test.
|
@ramesh-db Thanks for the review! Do you have a code pointer for this repo's testing framework? |
|
Hi @abrassel , you can checkout the file BaseTableCRUDTest and SdkTableCRUDTest. Ideally we would have wanted to add this check directly to the BaseTableCRUDTest, however we have currently kept the return type of |
|
Hi @ramesh-db I added a test. We should be good to go now. |
* start of login page * env and google auth button * merge with main, remove params reference * start of okta auth * initial commit for handling auth token (#67) * start of login with keycloak * handle google sign in with token * more google auth * profile dropdown * merge with main * merge with main * convert to axios * start of readme instructions * get current user endpoint (#70) clean up some other endpoints * commenting out UI until repositories are merged * clean up current user (#74) * yarn lock file * remove keycloak for now, node version error in jwt-decode dependency * commit yarn lock * remove state as useEffect dependency, comment out currentUser call for now --------- Co-authored-by: Xiang Xu <xiang.xu@databricks.com>
* start of login page * env and google auth button * merge with main, remove params reference * start of okta auth * initial commit for handling auth token (#67) * start of login with keycloak * handle google sign in with token * more google auth * profile dropdown * merge with main * merge with main * convert to axios * start of readme instructions * get current user endpoint (#70) clean up some other endpoints * commenting out UI until repositories are merged * clean up current user (#74) * yarn lock file * remove keycloak for now, node version error in jwt-decode dependency * commit yarn lock * remove state as useEffect dependency, comment out currentUser call for now --------- Co-authored-by: Xiang Xu <xiang.xu@databricks.com>
* start of login page * env and google auth button * merge with main, remove params reference * start of okta auth * initial commit for handling auth token (unitycatalog#67) * start of login with keycloak * handle google sign in with token * more google auth * profile dropdown * merge with main * merge with main * convert to axios * start of readme instructions * get current user endpoint (unitycatalog#70) clean up some other endpoints * commenting out UI until repositories are merged * clean up current user (unitycatalog#74) * yarn lock file * remove keycloak for now, node version error in jwt-decode dependency * commit yarn lock * remove state as useEffect dependency, comment out currentUser call for now --------- Co-authored-by: Xiang Xu <xiang.xu@databricks.com>
PR Checklist
Description of changes
Addresses #21
When performing a tables.list operation, it seems that the server returns the empty string "" instead of None as documented in the spec.
Fixed this by making the paginated request return
nullinstead.Testing
On a default run of unity catalog, did this manual test:
Any tips for testing this effectively?