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
feat: add factory option to BalancedPool #1237
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1237 +/- ##
==========================================
+ Coverage 93.85% 94.03% +0.18%
==========================================
Files 43 43
Lines 4052 4057 +5
==========================================
+ Hits 3803 3815 +12
+ Misses 249 242 -7
Continue to review full report at Codecov.
|
ec67852
to
24945d7
Compare
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.
Can you add a test for the factory actually being used?
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.
Also I see a problem here. The factory options can overlap with the constructor options. Not sure how to resolve that. I think we might have similar issues in the other agents as well. @mcollina wdyt?
Sure can do that. |
Could you add the option to our docs? I'm ok with the behavior - I assume the factory can override any of the agent options. |
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.
lgtm
* feat: add factory option to BalancedPool * add tests for balanced-pool with factory option * update docs for balanced-pool with factory option
* feat: add factory option to BalancedPool * add tests for balanced-pool with factory option * update docs for balanced-pool with factory option
* feat: add factory option to BalancedPool * add tests for balanced-pool with factory option * update docs for balanced-pool with factory option
closes #1220