-
Notifications
You must be signed in to change notification settings - Fork 1
FEAT: Add Executor factory #16
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
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR refactors the executor instantiation process by introducing an ExecutorFactory, which leverages a registration decorator to map executor configuration types to their respective executor implementations, replacing the original if/else logic. It also updates the ExecutorBase constructor signature to support an optional connectors parameter. Updated Class Diagram for Executor Factory and ExecutorsclassDiagram
class ExecutorFactory {
- _registry: dict[Type[ExecutorConfigBase], Type[ExecutorBase]]
+ register_executor(config_type: Type[ExecutorConfigBase]) : decorator
+ create_executor(strategy: Any, config: ExecutorConfigBase, update_interval: float) : ExecutorBase
}
class ExecutorBase {
+ __init__(strategy: ScriptStrategyBase, config: ExecutorConfigBase, update_interval: float, connectors: List[str]?)
+ start() : void
}
class ArbitrageExecutor {
}
class DCAExecutor {
}
class GridExecutor {
}
class PositionExecutor {
}
class TWAPExecutor {
}
class XEMMExecutor {
}
ExecutorFactory ..> ExecutorBase : creates
ExecutorBase <|-- ArbitrageExecutor
ExecutorBase <|-- DCAExecutor
ExecutorBase <|-- GridExecutor
ExecutorBase <|-- PositionExecutor
ExecutorBase <|-- TWAPExecutor
ExecutorBase <|-- XEMMExecutor
note for ExecutorFactory "Uses registration decorators to map ExecutorConfig types to Executor implementations."
File-Level Changes
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 @MementoRC - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a check in ExecutorFactory.register_executor to prevent accidental overwrites of an already registered executor for a given config type.
- Ensure that the ordering and usage of parameters (like connectors) in ExecutorBase.init remain consistent across all executor implementations to avoid positional argument issues.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 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 and I'll use the feedback to improve your reviews.
Before submitting this PR, please make sure:
A description of the changes proposed in the pull request:
Tests performed by the developer:
Tips for QA testing:
Summary by Sourcery
New Features: