Skip to content

Commit

Permalink
Remove None as default option from feature flags.
Browse files Browse the repository at this point in the history
To simplify the feature flag interface, default values can now only be
True or False. Implementations that need similar functionality can
implement separate methods, such as how `CachingFeatureFlagRouter` now
has a `feature_is_cached` method.

Related #98
  • Loading branch information
aholmes committed Aug 8, 2024
1 parent f87bb76 commit 5a374e7
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 44 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ def _notify_change(
else:
self._logger.warn(f"Setting new feature flag '{name}' to `{new_value}`.")

def _validate_name(self, name: str):
if type(name) != str:
raise TypeError("`name` must be a string.")

if not name:
raise ValueError("`name` parameter is required and cannot be empty.")

@override
def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
"""
Expand All @@ -37,43 +44,35 @@ def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
:param str name: The feature flag to check.
:param bool is_enabled: Whether the feature flag is to be enabled or disabled.
"""
if type(name) != str:
raise TypeError("`name` must be a string.")
self._validate_name(name)

if type(is_enabled) != bool:
raise TypeError("`is_enabled` must be a boolean.")

if not name:
raise ValueError("`name` parameter is required and cannot be empty.")

self._notify_change(name, is_enabled, self._feature_flags.get(name))

self._feature_flags[name] = is_enabled

return super().set_feature_is_enabled(name, is_enabled)

@override
def feature_is_enabled(
self, name: str, default: bool | None = False
) -> bool | None:
def feature_is_enabled(self, name: str, default: bool = False) -> bool:
"""
Determine whether a feature flag is enabled or disabled.
Subclasses should call this method to validate parameters and use cached values.
:param str name: The feature flag to check.
:param bool | None default: If the feature flag is not in the in-memory dictionary of flags,
:param bool default: If the feature flag is not in the in-memory dictionary of flags,
this is the default value to return. The default parameter value
when not specified is `False`.
:return bool | None: If `True`, the feature is enabled. If `False` or `None`, the feature is disabled.
:return bool: If `True`, the feature is enabled. If `False`, the feature is disabled.
"""
if type(name) != str:
raise TypeError("`name` must be a string.")
self._validate_name(name)

if not name:
raise ValueError("`name` parameter is required and cannot be empty.")
return self._feature_flags.get(name, default)

if name in self._feature_flags:
return self._feature_flags[name]
def feature_is_cached(self, name: str):
self._validate_name(name)

return default
return name in self._feature_flags
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,8 @@ def feature_is_enabled(self, name: str, check_cache: bool = True) -> bool: # ty
check_cache: Whether to use the cached value if it is cached. The default is `True`.
If the cache is not checked, the new value pulled from the database will be cached.
"""
if check_cache:
enabled = super().feature_is_enabled(name, None)
if enabled is not None:
return enabled
if check_cache and super().feature_is_cached(name):
return super().feature_is_enabled(name)

feature_flag = (
self._session.query(self._feature_flag)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,11 @@ def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
"""

@abstractmethod
def feature_is_enabled(
self, name: str, default: bool | None = False
) -> bool | None:
def feature_is_enabled(self, name: str, default: bool = False) -> bool:
"""
Determine whether a feature flag is enabled or disabled.
:param str name: The name of the feature flag.
:param bool | None default: A default value to return for cases where a feature flag may not exist. Defaults to False.
:return bool | None: If `True`, the feature is enabled. If `False` or `None`, the feature is disabled.
:param bool default: A default value to return for cases where a feature flag may not exist. Defaults to False.
:return bool: If `True`, the feature is enabled. If `False`, the feature is disabled.
"""
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
from BL_Python.platform.feature_flag.caching_feature_flag_router import (
CachingFeatureFlagRouter,
)
from mock import MagicMock
from pytest import LogCaptureFixture
from typing_extensions import override

_FEATURE_FLAG_TEST_NAME = "foo_feature"

Expand Down Expand Up @@ -94,22 +94,14 @@ def test__feature_is_enabled__uses_cache(value: bool):
logger = logging.getLogger("FeatureFlagLogger")
caching_feature_flag_router = CachingFeatureFlagRouter(logger)

call_count = 0

class FakeDict(dict): # pyright: ignore[reportMissingTypeArgument]
@override
def __getitem__(self, key: Any) -> Any:
nonlocal call_count
call_count += 1
return super().__getitem__(key) # pyright: ignore[reportUnknownMemberType,reportUnknownVariableType]

caching_feature_flag_router._feature_flags = FakeDict() # pyright: ignore[reportPrivateUsage]

mock_dict = MagicMock()
caching_feature_flag_router._feature_flags = mock_dict # pyright: ignore[reportPrivateUsage]
caching_feature_flag_router.set_feature_is_enabled(_FEATURE_FLAG_TEST_NAME, value)

_ = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)
_ = caching_feature_flag_router.feature_is_enabled(_FEATURE_FLAG_TEST_NAME)

assert call_count == 2
assert mock_dict.get.call_count == 3


@pytest.mark.parametrize("enable", [True, False])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ def test__feature_is_enabled__checks_cache(
):
session_mock = mocker.patch("sqlalchemy.orm.session.Session")
feature_is_enabled_mock = mocker.patch(
"BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.feature_is_enabled"
"BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.feature_is_cached"
)
_ = mocker.patch(
"BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.set_feature_is_enabled"
Expand All @@ -199,7 +199,7 @@ def test__feature_is_enabled__sets_cache(
):
session_mock = mocker.patch("sqlalchemy.orm.session.Session")
feature_is_enabled_mock = mocker.patch(
"BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.feature_is_enabled"
"BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.feature_is_cached"
)
set_feature_is_enabled_mock = mocker.patch(
"BL_Python.platform.feature_flag.caching_feature_flag_router.CachingFeatureFlagRouter.set_feature_is_enabled"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ def set_feature_is_enabled(self, name: str, is_enabled: bool) -> None:
return super().set_feature_is_enabled(name, is_enabled)

@override
def feature_is_enabled(
self, name: str, default: bool | None = False
) -> bool | None:
def feature_is_enabled(self, name: str, default: bool = False) -> bool:
return super().feature_is_enabled(name, default)


Expand Down

0 comments on commit 5a374e7

Please sign in to comment.