Open robrap opened 1 year ago
Hah, I recall having this conversation at an edX Arch Lunch when I was an intern in 2017. Nobody could name a reason for keeping a separate FEATURES dict, especially considering that boolean flags already existed outside of FEATURES at that point.
In any event, I'm in favor of flattening, either as FEATURE_XXX
or even just as XXX
(i.e., name the settings the same as the dict keys) if there weren't any conflicts.
Thanks @kdmccormick. I'll also note that https://docs.openedx.org/projects/edx-platform/en/latest/references/featuretoggles.html is a new feature that helps make it less important how consistent we are.
I like clarity that leads to consistency, so I think it would be useful (through DEPR and/or ADR) to state that the FEATURES
dict is deprecated, if that is the decision.
Around naming, I think TOGGLE
would be better than FEATURE
, and could be an optional or recommended suffix that makes the setting name more clear.
Also, some use ENABLE
, ENABLED
, DISABLE
, or DISABLED
in the name. I find it makes reading code confusing, because we separately call an is_enabled()
method (or the new is_disabled()
method), and my brain has to do less mental gymnastics when I read XXX_TOGGLE.is_enabled()
or XXX_TOGGLE.is_disabled()
.
Maybe the naming recommendations would be better as an edx-toggles how-to (or part of an existing how-to), and here we really just need to clarify whether or not to add to the dict.
I like TOGGLE
because it clarifies that the variable a toggle, without the awkwardness of ENABLE_BLAH.is_enabled()
@kdmccormick: Some thoughts since we last wrote on the topic:
ENABLE_
or DISABLE_
as a prefix to indicate the default value in the name, and I've learned how that can both be very convenient, and sometimes misleading. For example, an ENABLE_XXX
toggle is typically defaulted off, and the toggle is used to enable the feature. Using the name TOGGLE_XXX
loses this indication, and one would need to rely on docs or code to determine its default. This may be more time consuming, but should also be more accurate. [I'm thinking out loud here, and potentially re-convincing myself of the usefulness of this naming convention.]ENABLE_
settings to default on, so I wouldn't consider that to be a naming convention worth preserving.FEATURES = vars()
😈 ?)I also don't have time to carry this out right now. @feanil , is Aximprovements looking for work? If so, is collapsing FEATURES something they'd be interested in?
A little more detail on the migration path I'm imagining... we'd collapse all of FEATURES
into the root of common.py. For backwards compatibility, we'd provide a magical FEATURES
dict for a few releases that acted as a passthrough to the settings module:
class _Features(dict):
def __init__(self, pass_through_to):
self.pass_through_to = pass_through_to
def __getitem__(self, key):
return self.pass_through_to[key]
def __setitem(self, key, value):
warnings.warn(f"FEATURES is deprecated. Instead of overriding FEATURES['{key}'], just override {key}")
self.pass_through_to[key] = value
FEATURES = _Features(pass_through_to=vars())
EDIT: Hm, this might not work, because pass_through_to
may be bound to the namespace of common.py rather than the namespace of whatever settings module that is reading from and writing to FEATURES
.
_TOGGLE
would be better as a suffix to enable alpha sorting with other settings related to a feature area, rather than with all toggles being sorted together?Examples:
FEATURES['ENABLE_OAUTH2_PROVIDER']
=> OAUTH2_PROVIDER_TOGGLE
(drops ENABLE)FEATURES['CERTIFICATES_HTML_VIEW']
=> CERTIFICATES_HTML_VIEW_TOGGLE
Linting would be simple if we just put an upper bound on the size of the dict. So, it is possible that we could decide, add linting, document a naming convention alternative, and iterate later with better automating the full transition when and if that gets prioritized.
@robrap You're right, linting is totally feasible here just by inspecting the length or keys of the FEATURES dict.
@robrap Regarding naming... I'm looking at common.py to see what other third-party apps do. It seems like the convention is to prefix with feature area, so I like that a lot (most of our own apps already do it too :). However, they don't suffix with _TOGGLE; rather, they just kinda describe the behavior that would occur if they were set to True. Some examples:
Given that, I lean away from suffixing with _TOGGLE, or with anything else for that matter. That does still leave the awkwardness of MY_APP_ENABLED.is_enabled()
... but could we work around that by defining __bool__
on the Toggle base class to equal is_enabled()
, allowing all Toggles to be checked with:
if MY_APP_ENABLED:
...
A nice property of this is that all toggles can be checked the same way, regardless of whether they are Django settings, Toggles wrapping Django settings, or Toggles wrapping Waffle Flags. For CourseWaffleFlags, we could even support a nice syntax like:
if MY_COURSE_SENSITIVE_APP_ENABLED.for_context(<course_key>):
...
where for_context
creates a one-off Toggle instance that is scoped to <course_key>
.
@kdmccormick: Good thoughts. I think your proposal is a reasonable alternative, or enhancement, to what is captured in https://github.com/openedx/edx-toggles/issues/290. Yours is a more elegant solution from a readability perspective, as long as people don't get caught up wondering how it actually works.
@robrap Oh, I didn't realize that there was edx-toggles work in flight--nice!
Yours is a more elegant solution from a readability perspective, as long as people don't get caught up wondering how it actually works.
It's the classic tension between Having A Simple API and Having A Simple Implementation :)
Alternatively, in the spirit of There's One Way To Do It, I'm also OK with just having is_enabled()
, even if it's a mouthful sometimes, and a brain-twister other times. We could document that avoiding negative setting names is a best practice. For example, ENABLE_LEGACY_EXPERIENCE
is superior to DISABLE_NEW_EXPERIENCE
.
Anyway, that's a lot of opinions from me, all loosely held. In the end, my only strong opinion is that we need to have fewer layers between what the site operator configures and what the application sees.
Ultimately this should result in an ADR and/or a DEPR, but I'm first creating an issue to initiate discussion on the topic.
Background, I've heard in the past that we should stop using the
FEATURES
dict, but I do not know all the reasons or the recommended alternative. As new settings are added, it would be great if there was clarity around this.The following ADR on managing django settings mentions
FEATURES
, but it does not take any stance on this topic.Open questions:
FEATURE_XXX
?If anyone with history on this topic could shed some additional light, that would be great.