-
Notifications
You must be signed in to change notification settings - Fork 96
Add describe, delete, and list namespaces (REST) #507
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
…t idx.<namespace_op>
…y and not idx.<namespace_op>" This reverts commit 6a011e8.
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.
LGTM overall, left some comments and a few questions. Nice work, I'll defer to @jhamon for now on a lot of this as I'm still getting familiar with the client refactor.
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.
A couple things:
Require kwargs for new methods
I would add the @require_kwargs
decorator on all new methods. Search the repo for other examples of this. We haven't added it in 100% of places because it would be a breaking change to add it to methods that never had it before if people are successfully calling methods using positional arguments, but we want to use this on all new methods going forward.
Why? Well, python is flexible in how methods are called. If you had a function like this:
def mymethod(foo):
print(foo)
then you could call the function with positional arguments like mymethod('bar')
or kwarg-style as mymethod(foo='bar')
. Relying on positional arguments can cause problems down the road when new arguments are added, the args list gets long, if arguments transition from required to optional, etc. If everyone uses the keyword form from the beginning, a lot of these changes can be made without disrupting existing code.
Testing docs change
Unfortunately testing these docs changes is slightly involved because there's a problem preventing us from directly having sphinx as a dev-dependency. If you want to try building the docs, you need to follow the steps in this github action but do not commit any changes to the python version or poetry.lock when you are done.
Basically, you need to
poetry env use 3.11
to make sure you're using python 3.11- (Temporarily, do not commit) edit the pyproject.toml to require python
^3.11
poetry install -E asyncio -E grpc
- Install docs deps
poetry add sphinx myst-parser --group dev
- Build the docs
poetry run sphinx-build -b html docs docsbuild
- Inspect the output
open docsbuild/index.html
You will need to edit docs/rest.rst
and docs/asyncio.rst
to explicitly expose docs for your new methods, and then you'll want to look at them to see whether the generated text looks correct from a formatting standpoint.
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.
I left a ton of comments (sorry lol), but overall I think you did a nice job.
Problem
Add support for describe, delete, and and list namespaces for index and async index classes.
Solution
Following methods were added to the index and async index classes:
- list_namespaces(self, **kwargs) -> Iterator[ListNamespacesResponse] (for index.py)
- list_namespaces(self, **kwargs) -> AsyncIterator[ListNamespacesResponse] (for index_asyncio.py)
self, limit: Optional[int] = None, pagination_token: Optional[str] = None, **kwargs
) -> ListNamespacesResponse
Type of Change
Test Plan
Added integration tests for both index and async index classes