moved the store position to HandleOrderEvent so that the positions ar…#56
moved the store position to HandleOrderEvent so that the positions ar…#56
Conversation
…e continuously saved on OrderEvents
WalkthroughThe changes involve modifications to the position storage mechanism in the order handling and algorithm execution processes. The Changes
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Initialization/HandleOrderEvents.py (2)
44-44: Remove unnecessary empty line.This empty line creates extra spacing without adding readability.
45-46: Consider performance impact of frequent position storage.The position storage is called on every order event in live mode. While this ensures continuous position tracking, it might impact performance if there are many rapid order events.
Consider adding a debounce mechanism or batch processing for position storage to optimize performance while maintaining data consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Initialization/HandleOrderEvents.py(1 hunks)main.py(0 hunks)
💤 Files with no reviewable changes (1)
- main.py
🧰 Additional context used
🪛 Ruff (0.8.2)
Initialization/HandleOrderEvents.py
37-37: PositionsStore may be undefined, or defined from star imports
(F405)
🔇 Additional comments (1)
Initialization/HandleOrderEvents.py (1)
37-37: 🛠️ Refactor suggestionAdd explicit import for
PositionsStore.The
PositionsStoreclass is not explicitly imported. While it might be available through star imports, it's better to make dependencies explicit for better code maintainability and clarity.Let's verify the
PositionsStoreclass existence and location:✅ Verification successful
Add explicit import for
PositionsStorethrough the Tools packageUpdate the Tools import line to include PositionsStore:
from Tools import Logger, Helper, PositionsStore🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for PositionsStore class definition rg -l "class\s+PositionsStore" # Search for PositionsStore imports rg "from.*import.*PositionsStore|import.*PositionsStore"Length of output: 354
Script:
#!/bin/bash # Check imports in HandleOrderEvents.py rg "^from|^import" Initialization/HandleOrderEvents.pyLength of output: 149
🧰 Tools
🪛 Ruff (0.8.2)
37-37:
PositionsStoremay be undefined, or defined from star imports(F405)
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
+ Coverage 71.12% 71.33% +0.20%
==========================================
Files 30 30
Lines 2691 2693 +2
==========================================
+ Hits 1914 1921 +7
+ Misses 777 772 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/specs/initialization/handle_order_events_spec.py (2)
93-113: Add test documentation to clarify position storage triggers.While the test correctly verifies position storage in live mode, it would be more maintainable to document the specific conditions that trigger position storage (e.g., order status, live mode state).
Add a descriptive comment before the test:
with it('stores positions in live mode'): + # Verify that positions are stored when: + # 1. Algorithm is in LiveMode + # 2. Order is filled (Status=OrderStatus.Filled) + # 3. Position is found from the order event
115-135: Consider reducing test setup duplication.The setup code for position and order mocking is duplicated between live mode and backtest mode tests. Consider extracting common setup into helper methods.
Example refactor:
def setup_position_and_order(self): position = MagicMock( orderTag="TEST_POS", legs=[MagicMock(symbol=self.order_event.Symbol)] ) mock_order = MagicMock() mock_order.Tag = "TEST_POS" self.algorithm.Transactions.GetOrderById.return_value = mock_order self.handler.getPositionFromOrderEvent = MagicMock( return_value=(position, None, "close", mock_order) ) return position, mock_orderThen use it in both tests:
with it('stores positions in live mode'): self.algorithm.LiveMode = True setup_position_and_order(self) self.handler.Call() self.algorithm.positions_store.store_positions.assert_called_once()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Initialization/HandleOrderEvents.py(1 hunks)Tests/specs/initialization/handle_order_events_spec.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Initialization/HandleOrderEvents.py
🔇 Additional comments (2)
Tests/specs/initialization/handle_order_events_spec.py (2)
39-39: LGTM! Good test setup practice.Initializing
positions_storemock inbefore.eachensures consistent test state across all test cases.
93-158: Consider adding test coverage for position storage order of operations.The current tests verify that position storage occurs in live mode, but don't verify when it happens in relation to other operations (e.g., before/after updating order stats, handling assignments, etc.). Consider adding tests to verify the correct sequence of operations.
Example test case to add:
with it('stores positions after handling the order'): self.algorithm.LiveMode = True position, mock_order = setup_position_and_order(self) # Track operation sequence operation_sequence = [] position.updateOrderStats.side_effect = lambda: operation_sequence.append('update_stats') self.algorithm.positions_store.store_positions.side_effect = \ lambda: operation_sequence.append('store_positions') self.handler.Call() # Verify store_positions is called after updating order stats expect(operation_sequence).to(equal(['update_stats', 'store_positions']))
| with it('handles missing positions_store in live mode'): | ||
| # Setup LiveMode but remove positions_store | ||
| self.algorithm.LiveMode = True | ||
| # No need to remove positions_store as it doesn't exist by default | ||
|
|
||
| # Setup mock position and order | ||
| position = MagicMock( | ||
| orderTag="TEST_POS", | ||
| legs=[MagicMock(symbol=self.order_event.Symbol)] | ||
| ) | ||
| mock_order = MagicMock() | ||
| mock_order.Tag = "TEST_POS" | ||
| self.algorithm.Transactions.GetOrderById.return_value = mock_order | ||
|
|
||
| self.handler.getPositionFromOrderEvent = MagicMock( | ||
| return_value=(position, None, "close", mock_order) | ||
| ) | ||
|
|
||
| # Should not raise an error | ||
| self.handler.Call() | ||
| # Test passes if no exception is raised | ||
|
|
There was a problem hiding this comment.
Fix incorrect test implementation for missing positions_store scenario.
The test claims to verify behavior when positions_store is missing, but positions_store is always initialized in before.each. This means the test isn't actually testing the intended scenario.
To properly test this scenario:
with it('handles missing positions_store in live mode'):
# Setup LiveMode but remove positions_store
self.algorithm.LiveMode = True
- # No need to remove positions_store as it doesn't exist by default
+ # Remove positions_store to test the scenario
+ delattr(self.algorithm, 'positions_store')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with it('handles missing positions_store in live mode'): | |
| # Setup LiveMode but remove positions_store | |
| self.algorithm.LiveMode = True | |
| # No need to remove positions_store as it doesn't exist by default | |
| # Setup mock position and order | |
| position = MagicMock( | |
| orderTag="TEST_POS", | |
| legs=[MagicMock(symbol=self.order_event.Symbol)] | |
| ) | |
| mock_order = MagicMock() | |
| mock_order.Tag = "TEST_POS" | |
| self.algorithm.Transactions.GetOrderById.return_value = mock_order | |
| self.handler.getPositionFromOrderEvent = MagicMock( | |
| return_value=(position, None, "close", mock_order) | |
| ) | |
| # Should not raise an error | |
| self.handler.Call() | |
| # Test passes if no exception is raised | |
| with it('handles missing positions_store in live mode'): | |
| # Setup LiveMode but remove positions_store | |
| self.algorithm.LiveMode = True | |
| # Remove positions_store to test the scenario | |
| delattr(self.algorithm, 'positions_store') | |
| # Setup mock position and order | |
| position = MagicMock( | |
| orderTag="TEST_POS", | |
| legs=[MagicMock(symbol=self.order_event.Symbol)] | |
| ) | |
| mock_order = MagicMock() | |
| mock_order.Tag = "TEST_POS" | |
| self.algorithm.Transactions.GetOrderById.return_value = mock_order | |
| self.handler.getPositionFromOrderEvent = MagicMock( | |
| return_value=(position, None, "close", mock_order) | |
| ) | |
| # Should not raise an error | |
| self.handler.Call() | |
| # Test passes if no exception is raised |
2 participants
…e continuously saved on OrderEvents
Summary by CodeRabbit
New Features
Bug Fixes
Tests
HandleOrderEventsclass to verify behavior under differentLiveModeconditions.