-
Notifications
You must be signed in to change notification settings - Fork 5
Search Configuration API #1666
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
base: main
Are you sure you want to change the base?
Search Configuration API #1666
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
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 few adjustments. I think we should also ask if there is delete
endpoint, as supporting this in Toolkit will be difficult without delete.
@overload | ||
def __call__(self) -> Iterator[SearchConfig]: ... | ||
|
||
@overload | ||
def __call__(self, chunk_size: int) -> Iterator[SearchConfigList]: ... | ||
|
||
def __call__(self, chunk_size: int | None = None) -> Iterator[SearchConfig] | Iterator[SearchConfigList]: | ||
"""Iterate over configurations. | ||
Args: | ||
chunk_size: The number of configurations to return in each chunk. None will return all configurations. | ||
|
||
Yields: | ||
SearchConfig or SearchConfigList | ||
|
||
""" | ||
return iter(self.list()) | ||
|
||
def __iter__(self) -> Iterator[SearchConfig]: | ||
return self.__call__() |
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.
This endpoint does not support pagination, so lets skip this
@overload | |
def __call__(self) -> Iterator[SearchConfig]: ... | |
@overload | |
def __call__(self, chunk_size: int) -> Iterator[SearchConfigList]: ... | |
def __call__(self, chunk_size: int | None = None) -> Iterator[SearchConfig] | Iterator[SearchConfigList]: | |
"""Iterate over configurations. | |
Args: | |
chunk_size: The number of configurations to return in each chunk. None will return all configurations. | |
Yields: | |
SearchConfig or SearchConfigList | |
""" | |
return iter(self.list()) | |
def __iter__(self) -> Iterator[SearchConfig]: | |
return self.__call__() |
def __iter__(self) -> Iterator[SearchConfig]: | ||
return self.__call__() | ||
|
||
def update(self, configuration_update: SearchConfigWrite) -> SearchConfig: |
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.
def update(self, configuration_update: SearchConfigWrite) -> SearchConfig: | |
def upsert(self, configuration_update: SearchConfigWrite) -> SearchConfig: |
|
||
|
||
@pytest.fixture(scope="session") | ||
def existing_search_config(toolkit_client: ToolkitClient) -> dict: |
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.
Why not return a SearchConfig
here?
def existing_search_config(toolkit_client: ToolkitClient) -> dict: | |
def existing_search_config(toolkit_client: ToolkitClient) -> SearchConfig: |
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 also wanted count of already present configs thus was returning in dict. Could use tuple instead. with first element as count and second as SearchConfig. id can be fetched using SearchConfig.id
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.
since not explicitly testing list() method so will just return SearchConfig
def test_list_search_configurations(self, toolkit_client: ToolkitClient, existing_search_config: dict) -> None: | ||
"""Test that we can list search configurations.""" | ||
search_configs = toolkit_client.search_configurations.list() | ||
assert isinstance(search_configs, SearchConfigList) | ||
assert len(search_configs) == existing_search_config["total_configs"] |
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 think this is already tested in the fixture
The base branch was changed.
Description
Stacked on #1651
This PR introduces to search configurations API client to utilise list and update APIs for configurations. This will be used by Configurations resources to load search config resources and validate it.
Changelog