search

uclahs-cds / Ligare

GNU General Public License v2.0
0 stars 0 forks source link

Why does `_feature_flags` exist in the base class of `FeatureFlagRouter`? #95

Closed aholmes closed 2 months ago

aholmes commented 2 months ago

FeatureFlagRouter(ABC) contains a private instance member named _feature_flags that is not defined. Why does it exist? What is its purpose?

Related - why do we log this warning?

aholmes commented 2 months ago

As commented here

Subclasses should call this method to validate parameters and cache values.

and based on how self._feature_flags is used, this is base functionality to primitively cache values. It is an optional feature for extensions to take advantage of should they need it.

We may want to redesign this so that the abstract class only provides abstract methods, and we can provide a base "caching feature flag router" or similar that has this primitive caching, that other extensions can build on. However, it is also possible for extensions to not call the base class methods, which is allowed.

Regarding the warning, this was done:

  1. To notify that a feature flag's value is changing during runtime, which is a note-worthy event due to the enablement or disablement of features in code
  2. Is set as a warning to more likely ensure that this ends up in logs; info might not be logged due to noise, but runtime warnings are not often ignored
  3. To give a point-in-time when a feature was enabled or disabled

We may want to not rely on this, and let extensions of FeatureFlagRouter determine when, how, where, and whether to notify of this event. Perhaps add an abstract notify() method for base classes to implement?