refactor: flatten the methods in Client
#1914
Conversation
| self, | ||
| params: types.PaginatedRequestParams | None = None, | ||
| ) -> types.ListResourcesResult: | ||
| async def list_resources(self, *, cursor: str | None = None) -> types.ListResourcesResult: |
There was a problem hiding this comment.
This PR started from this. I understand that in the typescript world this is the best practice, but it's not in Python.
| # TODO(Marcelo): When do `raise_exceptions=True` actually raises? | ||
| raise_exceptions: bool = False, |
There was a problem hiding this comment.
I couldn't test this. What it is supposed to raise?
| timeout = request_read_timeout_seconds | ||
| elif self._session_read_timeout_seconds is not None: # pragma: no cover | ||
| timeout = self._session_read_timeout_seconds | ||
| timeout = request_read_timeout_seconds or self._session_read_timeout_seconds |
| addopts = """ | ||
| --color=yes | ||
| --capture=fd | ||
| --numprocesses auto |
There was a problem hiding this comment.
I actually forgot to remove this when I added the scripts/test script.
You need to be able to run pytest without -n auto, because you may want to run a single test, and you don't need multiple cores for it.
There was a problem hiding this comment.
@maxisbey @felixweinberger please check the changes in this file.
Kludex
changed the title
ClientClient
| self, | ||
| params: types.PaginatedRequestParams | None = None, | ||
| ) -> types.ListResourcesResult: | ||
| async def list_resources(self, *, cursor: str | None = None) -> types.ListResourcesResult: |
There was a problem hiding this comment.
will need to update migration docs
There was a problem hiding this comment.
Client is a new class, it doesn't need any migration docs.
| caps = client.server_capabilities | ||
| assert caps is not None | ||
| assert caps.tools is not None | ||
| assert client.server_capabilities == snapshot( |
There was a problem hiding this comment.
never seen this before, that's cool
| """Send a notification that the roots list has changed.""" | ||
| await self.session.send_roots_list_changed() | ||
| # TODO(Marcelo): Currently, there is no way for the server to handle this. We should add support. | ||
| await self.session.send_roots_list_changed() # pragma: no cover |
There was a problem hiding this comment.
why is there a new pragma?
There was a problem hiding this comment.
Because the test was not testing anything, and the server actually doesn't do anything with this notification. We need to solve the TODO, which will result in dropping the pragma.
| async def __aexit__(self, exc_type: type[BaseException] | None, exc_val: BaseException | None, exc_tb: Any) -> None: | ||
| """Exit the async context manager.""" | ||
| if self._exit_stack: | ||
| if self._exit_stack: # pragma: no branch |
There was a problem hiding this comment.
Because I don't think it's necessary to test this. If you can suggest a cute test, please push it.
No description provided.