-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: Implement create/get/delete/list table metadata methods #28
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.
Please change your title to adhere to the Angular style
i.e. "feat: Implement create/get/delete table metadata methods"
It looks like there is also a typo in your existing title 😆
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.
Some test ideas:
- Validate we deserialize timestamps correctly by asserting the created_at on the table metadata. We would probably need some kind of threshold for allowed values given that the time on the server may not match the time on the client.
- Retrieve metadata for a valid table ID that doesn't exist (like
000000000000000000000000
) and verify we raise the correct exception that includes the error details like the code and args. - Similarly, we could have an error case for the create and delete APIs.
@spanglerco @cameronwaterman I was working off this branch and since the PR is still up, I went ahead and added the list_tables method as well. I'll reset the reviews so y'all can take a look. New changes: 20f3bbf |
Co-authored-by: Paul Spangler <[email protected]>
What does this Pull Request accomplish?
Implements a set of table metadata routes:
Adds a custom serializer for our Pydantic models. This is so we can set certain options like
by_alias
.Switches attribute comments to use docstrings, because some of them wouldn't fit on a single line.
Why should this Pull Request be merged?
We need this functionality.
(This part of the PR template is awkward. I'm never sure what to put other than "because we need it")
What testing has been done?
Added our first functional test 🎉 Feel free to suggest other test cases. I think we'll want at least one that tests an expected error.