-
Notifications
You must be signed in to change notification settings - Fork 74
Fix event handler signatures for keep-alive connection events #49
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
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
…KeepAlive._emit_event() Fixes issue #43: Error in event handler for connected: AsyncPocketOptionClient._on_keep_alive_connected() takes 1 positional argument but 2 were given Changed: - _on_keep_alive_connected(self) -> _on_keep_alive_connected(self, data=None) - _on_keep_alive_reconnected(self) -> _on_keep_alive_reconnected(self, data=None) - _on_keep_alive_message(self, message) -> _on_keep_alive_message(self, data=None) with proper dict extraction Co-authored-by: theshadow76 <59869868+theshadow76@users.noreply.github.com>
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 PR fixes a critical bug where event handlers in AsyncPocketOptionClient had incorrect signatures, causing TypeError when called by ConnectionKeepAlive._emit_event(). The issue (#43) manifested as "takes 1 positional argument but 2 were given" errors because the handlers didn't accept the data parameter that _emit_event() always passes.
Key Changes:
- Updated three event handler signatures to accept a
data: Dict[str, Any] = Noneparameter - Modified
_on_keep_alive_messageto extract the message string from the data dictionary - Added comprehensive test suite to verify the fix and prevent regression
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pocketoptionapi_async/client.py | Fixed signatures of _on_keep_alive_connected, _on_keep_alive_reconnected, and _on_keep_alive_message to accept the data parameter passed by ConnectionKeepAlive._emit_event() |
| tests/test_event_handler_signatures.py | Added comprehensive test suite covering all signature fixes, including edge cases, integration tests, and callback propagation verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return candles | ||
|
|
||
| async def _on_keep_alive_connected(self): | ||
| async def _on_keep_alive_connected(self, data: Dict[str, Any] = None): |
Copilot
AI
Nov 29, 2025
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.
The type hint should be Optional[Dict[str, Any]] instead of Dict[str, Any] since the default value is None. This properly indicates that the parameter can be either a dictionary or None.
async def _on_keep_alive_connected(self, data: Optional[Dict[str, Any]] = None):| async def _on_keep_alive_connected(self, data: Dict[str, Any] = None): | |
| async def _on_keep_alive_connected(self, data: Optional[Dict[str, Any]] = None): |
| logger.error(f"Error in connected callback: {e}") | ||
|
|
||
| async def _on_keep_alive_reconnected(self): | ||
| async def _on_keep_alive_reconnected(self, data: Dict[str, Any] = None): |
Copilot
AI
Nov 29, 2025
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.
The type hint should be Optional[Dict[str, Any]] instead of Dict[str, Any] since the default value is None. This properly indicates that the parameter can be either a dictionary or None.
async def _on_keep_alive_reconnected(self, data: Optional[Dict[str, Any]] = None):| async def _on_keep_alive_reconnected(self, data: Dict[str, Any] = None): | |
| async def _on_keep_alive_reconnected(self, data: Optional[Dict[str, Any]] = None): |
| logger.error(f"Error in reconnected callback: {e}") | ||
|
|
||
| async def _on_keep_alive_message(self, message): | ||
| async def _on_keep_alive_message(self, data: Dict[str, Any] = None): |
Copilot
AI
Nov 29, 2025
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.
The type hint should be Optional[Dict[str, Any]] instead of Dict[str, Any] since the default value is None. This properly indicates that the parameter can be either a dictionary or None.
async def _on_keep_alive_message(self, data: Optional[Dict[str, Any]] = None):| async def _on_keep_alive_message(self, data: Dict[str, Any] = None): | |
| async def _on_keep_alive_message(self, data: Optional[Dict[str, Any]] = None): |
ConnectionKeepAlive._emit_event()passes adataargument to event handlers, but the client handlers didn't accept it, causingTypeError: _on_keep_alive_connected() takes 1 positional argument but 2 were given.Changes
_on_keep_alive_connected: Adddata: Dict[str, Any] = Noneparameter_on_keep_alive_reconnected: Adddata: Dict[str, Any] = Noneparameter_on_keep_alive_message: Change frommessagestring todatadict, extract message viadata.get("message", "")Fixes #43
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.