-
Notifications
You must be signed in to change notification settings - Fork 1
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!
@@ -5,7 +5,7 @@ | |||
|
|||
|
|||
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?