feat: ability to filter executions by tool key #406
Conversation
| if tool_key is not None: | ||
| import json | ||
|
|
||
| params["filter"] = json.dumps({"tool": {"toolManifest": {"key": tool_key}}}) |
There was a problem hiding this comment.
Pull request overview
This PR adds the ability to filter executions by tool key by introducing a new tool_key parameter to the executions.list() method. However, this change removes the existing filter parameter, which is a breaking change.
- Replaced the generic
filterparameter with a specifictool_keyparameter inexecutions.list() - Added JSON-based filter construction to map
tool_keyto the API's filter format - Added tests to verify the new filtering functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/platform/executions.py | Replaced filter parameter with tool_key and added logic to construct JSON filter for tool manifest key matching |
| tests/test_executions.py | Added new test file with tests for listing executions and filtering by tool key |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| params["filter"] = filter | ||
|
|
||
| if tool_key is not None: | ||
| import json |
There was a problem hiding this comment.
The json module should be imported at the top of the file with other imports (after line 10) rather than inside the function. This is inconsistent with Python best practices and the import style used in most other files in the codebase (e.g., src/auth.py, src/config.py, src/platform/client.py).
| page_size: int | None = None, | ||
| order: str | None = None, | ||
| filter: str | None = None, | ||
| tool_key: str | None = None, |
There was a problem hiding this comment.
This is a breaking API change. The filter parameter has been removed and replaced with tool_key. Existing code that uses the filter parameter (e.g., tests/test_tools_api.py:12 and src/platform/job.py:1647) will break with this change. Consider either keeping both parameters or providing a deprecation path.
tests/test_executions.py
Outdated
| data = client.executions.list(tool_key="deeporigin.bulk-docking") | ||
| executions = data.get("data", []) | ||
| assert isinstance(executions, list), "Expected a list" | ||
| assert len(executions) > 0, "Expected at least one execution" |
There was a problem hiding this comment.
The test assumes that the API will return at least one execution with the tool key "deeporigin.bulk-docking". This assumption makes the test fragile - it will fail if no executions exist for this tool. Consider using a mock or creating test data, or handle the case where no executions exist more gracefully.
| if tool_key is not None: | ||
| import json | ||
|
|
||
| params["filter"] = json.dumps({"tool": {"toolManifest": {"key": tool_key}}}) |
There was a problem hiding this comment.
The filter construction uses hardcoded nested structure without validation. If tool_key is an empty string or contains special characters that need escaping in JSON, this could lead to unexpected behavior. Consider adding validation for the tool_key parameter before constructing the filter.
No description provided.