-
Notifications
You must be signed in to change notification settings - Fork 2
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
🎨 and ♻️ ccxt execute_order refactoring #513
Conversation
Reviewer's Guide by SourceryThis pull request refactors the CCXT execute_order function and makes minor improvements to documentation and code structure across multiple files in the cefi/handler directory. The main changes focus on simplifying the execute_order function, improving error handling, and updating import statements. File-Level Changes
Tips
|
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 @mraniki - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider removing the commented-out code in ccxt.py if it's no longer needed. If it's being kept for reference, add a comment explaining why.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
except Exception as e: | ||
logger.error("{} Error {}", self.name, e) | ||
return f"Error executing {self.name}" | ||
|
||
# async def execute_order(self, order_params): |
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.
suggestion: Consider removing large blocks of commented-out code
While keeping the old version for reference can be helpful, large blocks of commented code can clutter the file. Consider using version control for this purpose instead.
# async def execute_order(self, order_params): | |
# Remove this commented-out method declaration |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #513 +/- ##
==========================================
- Coverage 69.40% 66.28% -3.13%
==========================================
Files 8 9 +1
Lines 487 519 +32
==========================================
+ Hits 338 344 +6
- Misses 149 175 +26 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
Refactor the
execute_order
method in theCcxtHandler
class for improved clarity and efficiency, update method descriptions in theCexClient
class, and adjust import paths across various handler files to align with the new module structure.Enhancements:
execute_order
method inCcxtHandler
to improve code readability and maintainability by removing redundant code and simplifying logic.CexClient
class to streamline method descriptions and remove unnecessary details about the CCXT library.Chores:
.client
to._client
across multiple handler files to reflect the updated module structure.