-
Notifications
You must be signed in to change notification settings - Fork 2
feat: allow setting bandits initial configuration #72
base: main
Are you sure you want to change the base?
Conversation
f6651b6 to
5059d8c
Compare
d69872e to
cf37b6a
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.
Looking good but let's cover these changes with automated tests!
|
|
||
| def test_init_valid(): | ||
| Configuration(flags_configuration='{"flags": {}}') | ||
| Configuration(flags_configuration=b'{"flags": {}}') |
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.
Since we're allowing bytes or a string, I imagine we should test both.
Also, let's have another test for initialization with a provided bandit configuration so we know it works!
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.
String is essentially deprecated and removed in 4.0.0
| def __init__( | ||
| self, | ||
| flags_configuration: Union[bytes, str], | ||
| bandits_configuration: Union[bytes, str, None] = None, |
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.
👍
|
Thanks, great stuff! Let's bump versions and publish after this change is in. |
labels: mergeable
Fixes: #issue
Motivation and Context
Description
Allow passing initial configuration for bandits.
How has this been tested?