Skip to content
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

process_pending_orders bug fix #471

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Conversation

grzesir
Copy link
Contributor

@grzesir grzesir commented Jun 12, 2024

No description provided.

Copy link
Contributor

korbit-ai bot commented Jun 12, 2024

My review is in progress 📖 - I will have feedback for you in a few minutes!

Copy link
Contributor

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed your code and found 1 potential issue. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.


Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.

Comment on lines +722 to 723
if process_pending_orders and hasattr(self.broker, "process_pending_orders"):
self.broker.process_pending_orders(strategy=self)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

category Objects and Data Structures

Using hasattr to check if self.broker has a process_pending_orders method can lead to unpredictable code and violates the 'tell, don't ask' principle. Consider defining a common interface or abstract base class for brokers that includes a process_pending_orders method. For brokers that don't need to process pending orders, you can implement this method as a no-op. This will make the code more predictable and maintainable.

@grzesir grzesir merged commit e565fe9 into dev Jun 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant