-
-
Notifications
You must be signed in to change notification settings - Fork 536
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
Strict graphql ws typed dicts #3689
Strict graphql ws typed dicts #3689
Conversation
Reviewer's Guide by SourceryThis PR refactors the GraphQL WebSocket protocol types to be more strictly typed using TypedDict with Literal and NotRequired types. The changes improve type safety and make the distinction between null and undefined values clearer while maintaining performance. Updated class diagram for GraphQL WebSocket protocol typesclassDiagram
class ConnectionInitMessage {
+Literal["connection_init"] type
+NotRequired[Dict[str, object]] payload
}
class StartMessagePayload {
+str query
+NotRequired[Dict[str, object]] variables
+NotRequired[str] operationName
}
class StartMessage {
+Literal["start"] type
+str id
+StartMessagePayload payload
}
class StopMessage {
+Literal["stop"] type
+str id
}
class ConnectionTerminateMessage {
+Literal["connection_terminate"] type
}
class ConnectionErrorMessage {
+Literal["connection_error"] type
+NotRequired[Dict[str, object]] payload
}
class ConnectionAckMessage {
+Literal["connection_ack"] type
}
class DataMessagePayload {
+object data
+NotRequired[List[GraphQLFormattedError]] errors
+NotRequired[Dict[str, object]] extensions
}
class DataMessage {
+Literal["data"] type
+str id
+DataMessagePayload payload
}
class ErrorMessage {
+Literal["error"] type
+str id
+GraphQLFormattedError payload
}
class CompleteMessage {
+Literal["complete"] type
+str id
}
class ConnectionKeepAliveMessage {
+Literal["ka"] type
}
class OperationMessage {
+ConnectionInitMessage
+StartMessage
+StopMessage
+ConnectionTerminateMessage
+ConnectionErrorMessage
+ConnectionAckMessage
+DataMessage
+ErrorMessage
+CompleteMessage
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @DoctorJohn - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Add runtime validation for required payload fields (link)
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 1 blocking issue
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Thanks for adding the Here's a preview of the changelog: In this release, all types of the legacy graphql-ws protocol were refactored. Here's the tweet text:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3689 +/- ##
==========================================
- Coverage 97.01% 97.01% -0.01%
==========================================
Files 503 502 -1
Lines 33550 33570 +20
Branches 5634 5634
==========================================
+ Hits 32548 32567 +19
Misses 796 796
- Partials 206 207 +1 |
CodSpeed Performance ReportMerging #3689 will not alter performanceComparing Summary
|
Description
This PR refactors all types of the legacy graphql-ws protocol. I continued using typed dicts (vs dataclasses) for performance reasons, simplicity, and the ability to model null vs undefined easily. In the linked issue I complained about them, but now that we have
NotRequired
andRequired
available for typing, they're fun to work with and enable the modelling of super precise types.Pylance is now pretty happy when I open the protocol implementation, some type casts were removed, and some minor type errors in tests surfaced and were fixed.
Types of Changes
Issues Fixed or Closed by This PR
Summary by Sourcery
Refactor the graphql-ws protocol types to use stricter typed dicts, enhancing type safety and clarity. Update tests to reflect these changes and add a release note to document the improvements.
Enhancements:
Documentation:
Tests: