Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

feat: allow setting bandits initial configuration #72

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rasendubi
Copy link
Collaborator



labels: mergeable

Fixes: #issue

Motivation and Context

Description

Allow passing initial configuration for bandits.

How has this been tested?

@rasendubi rasendubi force-pushed the feat-bandits-initial-configuration branch from f6651b6 to 5059d8c Compare September 27, 2024 12:25
@rasendubi rasendubi requested a review from aarsilv September 27, 2024 12:26
@rasendubi rasendubi force-pushed the feat-bandits-initial-configuration branch from d69872e to cf37b6a Compare September 27, 2024 12:31
Copy link
Contributor

@aarsilv aarsilv left a 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": {}}')
Copy link
Contributor

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!

Copy link
Collaborator Author

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@aarsilv aarsilv requested a review from schmit October 8, 2024 21:07
@schmit
Copy link
Contributor

schmit commented Oct 9, 2024

Thanks, great stuff! Let's bump versions and publish after this change is in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants