🥚 update capitalcom.py Modify parameters SL / TP of a position. Modi… #464
Conversation
…y parameters such as SL / TP of a position that is opened. No capability to modify amount to reduce size
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @mraniki - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| gsl=order_params.get("trailing_stop_enabled", False), | ||
| tsl=order_params.get("trailing_stop_enabled", False), |
There was a problem hiding this comment.
issue (bug_risk): Potential bug with trailing stop parameters.
Both 'gsl' and 'tsl' are set to the same value from 'trailing_stop_enabled'. If these should be different or configurable separately, this needs adjustment.
| @@ -350,3 +350,58 @@ async def execute_order(self, order_params): | |||
| except Exception as e: | |||
| logger.error("{} Error {}", self.name, e) | |||
| return f"Error executing {self.name}: {e}" | |||
|
|
|||
| async def modify_position(self, order_params): | |||
There was a problem hiding this comment.
suggestion (code_clarification): Method documentation could be more descriptive.
The method docstring should include more details about the parameters, especially what 'action' is used for, as it's included but not described.
| async def modify_position(self, order_params): | |
| async def modify_position(self, order_params): | |
| """ | |
| Modify parameters such as SL / TP of a position that is opened. | |
| Args: | |
| order_params (dict): A dictionary containing the parameters for the order modification. | |
| Expected keys include 'action' which describes the type of modification to apply. | |
| """ |
| logger.error(f"Error modifying order: {order['errorCode']}") | ||
| return str(order["errorCode"]) | ||
|
|
||
| logger.debug("Order: {}", order) |
There was a problem hiding this comment.
issue (code_refinement): Logger placeholder mismatch.
The logger uses '{}' as a placeholder, but this syntax requires '.format()' or should be changed to use f-string for consistency with other log statements.
| @@ -266,3 +266,21 @@ async def submit_order(self, order_params): | |||
| f"{client.name}:\n{await client.execute_order(order_params)}\n" | |||
| ) | |||
| return "\n".join(_order) | |||
|
|
|||
| async def modify_position(self, order_params): | |||
There was a problem hiding this comment.
suggestion (code_clarification): Inconsistent method documentation.
The documentation for 'modify_position' in 'main.py' is less detailed compared to its implementation in 'capitalcom.py'. Consider enhancing it for consistency and clarity.
| async def modify_position(self, order_params): | |
| async def modify_position(self, order_params): | |
| """ | |
| Modify the parameters of an existing trading position, such as stop loss (SL) and take profit (TP) values. | |
| Args: | |
| order_params (dict): A dictionary containing the parameters to be modified for the position. | |
| Returns: | |
| dict: The result of the modification request, typically containing status and any error messages. | |
| """ |
| async def modify_position(self, order_params): | ||
| """ | ||
| Modify parameters such as SL / TP of a position that is opened | ||
| No capability to modify amount to reduce | ||
|
|
||
| Args: | ||
| order_params (dict): | ||
| action(str) | ||
| instrument(str) | ||
| quantity(int) |
There was a problem hiding this comment.
issue (testing): Missing tests for the new modify_position method.
The PR introduces a new method modify_position which includes several branches and error handling, but there are no unit tests provided to verify the behavior of this method under various conditions, including normal operation and error scenarios. It's crucial to ensure that this functionality is covered by tests to prevent future regressions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #464 +/- ##
==========================================
- Coverage 71.59% 69.13% -2.47%
==========================================
Files 7 7
Lines 440 460 +20
==========================================
+ Hits 315 318 +3
- Misses 125 142 +17 ☔ View full report in Codecov by Sentry. |
…fy parameters such as SL / TP of a position that is opened. No capability to modify amount to reduce size