-
Notifications
You must be signed in to change notification settings - Fork 13
fix: Enhance settings processing to support nested dictionaries. #433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix: Enhance settings processing to support nested dictionaries. #433
Conversation
High-level feedback:
|
""" | ||
settings_dict_copy = {} | ||
default_attribute_value = object() # default is not a bool, so won't be included | ||
for attr in dir(settings): | ||
if not attr.startswith('__'): | ||
value = getattr(settings, attr, default_attribute_value) | ||
settings_dict_copy[attr] = value | ||
print("hello:settings_dict_copy", settings_dict_copy) |
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.
We need to remove this.
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.
Yes, got it. Was just for testing I will remove this.
Test that deeply nested dictionary-based toggles are properly extracted. | ||
""" | ||
|
||
def test_deeply_nested_event_bus_producer_config_extraction(self): |
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.
This test is really bloated, we should really simplify it to make sure that it's a unit test, not a full integration test. I fail to see the benefit of testing a dozen settings at once. Just a single nested dictionary with a single entry should suffice.
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.
Following the suggestion, created a single class with two functions checking level 1 nesting and level 4 nesting.
self.assertNotIn(non_boolean_key, settings_dict) | ||
|
||
|
||
if __name__ == '__main__': |
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.
Why is this needed?
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.
It was not needed so removed it.
@@ -0,0 +1,416 @@ | |||
""" |
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.
Do we really need to create a new test module? Wouldn't it make more sense to add unit tests to test_setting_toggles.py?
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.
Created the class within test_setting_toggles.py.
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.
It seems like other tests for state/internal/report.py
live in test_state.py
. I agree we don't want a new file, but maybe this is the right test file? You can see how to best incorporate the tests. But maybe I'm missing something.
d220a6f
to
9ef6749
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.
Thanks for the other updates. It looks much closer. Some minor improvements recommended.
""" | ||
Test that Level 1 nested boolean values are correctly extracted. | ||
|
||
Level 1: CONFIG['simple_toggle'] | ||
""" | ||
# Level 1 nesting: CONFIG['simple_toggle'] |
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.
Removing unnecessary comments. This is clear from the test itself.
""" | |
Test that Level 1 nested boolean values are correctly extracted. | |
Level 1: CONFIG['simple_toggle'] | |
""" | |
# Level 1 nesting: CONFIG['simple_toggle'] | |
""" | |
Test that Level 1 nested boolean values are correctly extracted. | |
""" |
_add_setting(settings_dict, nested_config, 'CONFIG') | ||
|
||
# Test Level 1 nesting | ||
level1_toggle = "CONFIG['simple_toggle']" |
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.
Nit: I like using expected
sometimes to make it more clear that this isn't being used for setup, but just as an expected value. This is just an opinion, so no change is necessary if you don't like the suggestion.
level1_toggle = "CONFIG['simple_toggle']" | |
expected_level1_toggle = "CONFIG['simple_toggle']" |
'advanced': { | ||
'settings': { | ||
'notifications': { | ||
'enabled': False |
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.
Because of how Python treats Falsey stuff, I think True will be a slightly better test.
'enabled': False | |
'enabled': True |
Fill the `settings_dict`: will only include values that are set to true or false. | ||
Now supports arbitrarily deep nested dictionaries with recursion. |
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.
- The first line of a docstring should be a 1-liner summary.
- I removed the word "Now". No one needs to know that it didn't support this in the past. I try to avoid temporal comments.
Fill the `settings_dict`: will only include values that are set to true or false. | |
Now supports arbitrarily deep nested dictionaries with recursion. | |
Fill the `settings_dict`: will only include values that are set to true or false. | |
Supports arbitrarily deep nested dictionaries with recursion. |
…nce docstring clarity
[request] In addition to the possible test move:
|
Problem Description
Boolean toggles nested deeply in Django settings dictionaries (3+ levels) were not being detected by the Toggle State API endpoint
/api/toggles/v0/state/
.Root Cause
The
_add_settings()
function inreport.py
only processed dictionaries one level deep, missing boolean values nested deeper in the configuration hierarchy.Fix
Enhanced the
_add_settings()
function with recursion to traverse arbitrarily deep nested dictionaries:_add_setting_recursively()
helper function.setting_dict_name()
function.Conclusion
Example:
Problem encountered after the fix: Duplicate Detection
The recursive fix is detecting boolean values from multiple copies of the same configuration that exist in different settings dictionaries.
For example:
Same boolean detected in 3 different places:
Further approach
Working to fix this till then it is suggested not to merge.
Open to suggestions regarding the fixes to the current problem.
TIcket
https://2u-internal.atlassian.net/browse/BOM2-24?atlOrigin=eyJpIjoiYTFhNzZkN2Q2ZWFiNDE5YTg4YTMxMTllYTQ2ZDE5OWEiLCJwIjoiaiJ9