fix(sdk): 按 Copilot 评论修正 Python SDK（URL 编码、JSON 异常、GeneManifest 类型、sa…#18
Conversation
…fe_dump、时间戳） Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR fixes several correctness issues in the Python SDK that were identified in a prior Copilot review. The changes cover URL encoding of query parameters, proper JSON exception handling, type accuracy of GeneManifest, safer YAML serialization, and a clean datetime import.
Changes:
- URL encoding: Replace manual query string construction (
f"{k}={v}") with aparamsdict passed tohttpx, which handles percent-encoding automatically. - Type correctness:
GeneManifestchanged fromtotal=False(all fields implicitly optional) tototal=TruewithNotRequired[...]for truly optional fields, correctly reflecting the required vs. optional fields of the TypeScript Zod schema. - Cleanup fixes:
json.JSONDecodeErrorhandling for non-JSON responses,yaml.safe_dumpinstead ofyaml.dump, and a cleandatetime/UTCimport replacing an inline__import__hack.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/sdk/python/src/genehub_sdk/types.py |
GeneManifest now uses total=True + NotRequired to distinguish required vs. optional fields; imports NotRequired |
packages/sdk/python/src/genehub_sdk/client.py |
Fixes URL encoding via params dict, adds json.JSONDecodeError guard, adds import json |
packages/sdk/python/src/genehub_sdk/learning/engine.py |
Replaces inline __import__ datetime hack with proper module-level from datetime import UTC, datetime |
packages/sdk/python/src/genehub_sdk/adapters/generic.py |
Switches yaml.dump to yaml.safe_dump to prevent arbitrary object serialization |
packages/sdk/python/README.md |
Corrects the search_genes usage example to use positional arg instead of wrong keyword q= |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| data = resp.json() | ||
| except json.JSONDecodeError: | ||
| msg = resp.text or f"HTTP {resp.status_code}" | ||
| raise GeneHubError(msg) from None |
There was a problem hiding this comment.
The new json.JSONDecodeError handling path (lines 41–45) is not covered by any test. The existing test file tests/test_client.py has tests for success responses and API error responses, but no test exercises the case where the server returns a non-JSON body (e.g., an HTML 502 page). A test using httpx_mock.add_response(content=b"<html>Bad Gateway</html>", status_code=502) and asserting that GeneHubError is raised would cover this branch.
Made-with: Cursor
…fe_dump、时间戳）
Made-with: Cursor