-
Notifications
You must be signed in to change notification settings - Fork 643
Consistently use Dataclasses in Python SDK #216
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?
Conversation
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Cross-SDK Consistency ReviewI've reviewed this PR for consistency with the other SDK implementations (Node.js/TypeScript, Go, and .NET). The Python SDK is converting response types from TypedDict to dataclasses, which is great for consistency within Python. However, I've identified one cross-SDK inconsistency to consider: ❌ Inconsistency Found:
|
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Cross-SDK Consistency Review ✅I've reviewed this PR for consistency across all four SDK implementations (Python, TypeScript, Go, and .NET). SummaryThis PR successfully aligns the Python SDK with the other SDKs by converting return types from raw dicts (TypedDict) to dataclasses. This is excellent for API consistency! Current State After This PR
Minor Inconsistency Noted (Not a blocker for this PR)The TypeScript SDK's Promise<{ message: string; timestamp: number; protocolVersion?: number }>While all other SDKs (including Python after this PR) use a named type like Recommendation✅ This PR maintains good cross-SDK consistency. The Python SDK now properly returns typed objects instead of raw dicts, matching the pattern used in Go and .NET. No changes needed to this PR. Great work on improving the Python SDK's type consistency!
|
✅ Cross-SDK Consistency Review: APPROVEDI've reviewed PR #216 for consistency across all four SDK implementations (Node.js/TypeScript, Python, Go, and .NET). This PR successfully addresses the Python SDK's inconsistency issue and brings it into alignment with the other SDKs. SummaryWhat Changed: The Python SDK was returning a mix of raw dictionaries (via Consistency Status: ✅ Excellent cross-SDK alignment Detailed Cross-SDK Comparison
Key Observations✅ Consistent approach: All SDKs now use strongly-typed response objects:
✅ API naming consistency: Method names follow language conventions while maintaining semantic parity:
✅ Field naming consistency: All SDKs use the same JSON field names:
Minor Inconsistency Found (Pre-existing)
The Go SDK doesn't implement the Recommendation: Consider adding ConclusionThis PR successfully achieves its goal of making the Python SDK consistent internally and aligned with the other SDK implementations. The changes are well-executed:
No cross-SDK consistency issues introduced by this PR. Great work! 🎉
|
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.
Pull request overview
This pull request converts several TypedDict response types to dataclasses in the Python SDK to improve type consistency and enable structured attribute access. The changes affect core response types returned by the Copilot CLI API.
Changes:
- Converted response types (PingResponse, StopError, GetStatusResponse, GetAuthStatusResponse, ModelInfo and related classes, SessionMetadata) from TypedDict to dataclass with from_dict/to_dict methods
- Updated all dictionary-style access patterns to attribute access throughout test files
- Removed unnecessary type: ignore comment and cast() call that are no longer needed
- Updated API documentation to reflect dataclass return types
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/copilot/types.py | Converted 11 TypedDict classes to dataclasses with serialization methods |
| python/copilot/client.py | Updated return types and added from_dict conversions for all API methods |
| python/copilot/init.py | Added PingResponse and StopError to public exports |
| python/e2e/test_session.py | Updated tests to use attribute access instead of dictionary access |
| python/e2e/test_client.py | Updated tests to use attribute access instead of dictionary access |
| python/copilot/tools.py | Removed unnecessary type: ignore comment |
Comments suppressed due to low confidence (1)
python/copilot/types.py:658
- The from_dict method should convert the optional summary field to str type when it is not None, similar to how the required string fields are converted. Currently, summary is passed through without type conversion, which could cause type inconsistencies if the server returns a non-string value.
summary=summary,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Cross-SDK Consistency ReviewThis PR improves cross-SDK consistency by converting the Python SDK from TypedDict to dataclasses for response types. Consistency AnalysisAfter reviewing all four SDK implementations, I can confirm this change aligns Python with the patterns used in other languages:
Key Improvements
Affected MethodsThe following methods now return typed dataclass instances instead of dictionaries:
No consistency issues found. This PR successfully brings the Python SDK into alignment with the type safety patterns used across all other SDK implementations.
|
As titled