-
Notifications
You must be signed in to change notification settings - Fork 7
Implement "last feature flag wins" behavior for duplicate feature flag IDs #59
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: main
Are you sure you want to change the base?
Conversation
Co-authored-by: mrm9084 <[email protected]>
Co-authored-by: mrm9084 <[email protected]>
…nique list generation Co-authored-by: mrm9084 <[email protected]>
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.
Pull Request Overview
Implements “last feature flag wins” resolution and deduplicated flag name listing to align with Microsoft Feature Management behavior.
- Core feature resolution now returns the last matching flag instead of the first.
- Flag name listing deduplicates IDs while preserving original order.
- Added sync and async tests covering duplicate-flag scenarios.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
featuremanagement/_featuremanagerbase.py | Updated _get_feature_flag to reverse-search flags; dedupe names in _list_feature_flag_names . |
tests/test_feature_manager.py | Added sync tests for duplicate-flag handling and mixed scenarios. |
tests/test_feature_manager_async.py | Added async tests for duplicate-flag handling (last-wins behavior). |
Comments suppressed due to low confidence (2)
tests/test_feature_manager.py:274
- [nitpick] If preserving order is important, you might assert exact list equality (
flag_names == expected_names
) instead of comparing sets to ensure the intended ordering is tested.
assert set(flag_names) == set(expected_names)
tests/test_feature_manager_async.py:222
- Consider adding a
test_duplicate_feature_flags_mixed_with_unique_async
async test to mirror the sync mixed-flag scenario and ensure consistency across both suites.
@pytest.mark.asyncio
index = len(feature_flags) - 1 | ||
|
||
while index >= 0: | ||
if feature_flags[index].get("id") == feature_flag_name: | ||
return FeatureFlag.convert_from_json(feature_flags[index]) | ||
index -= 1 |
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.
[nitpick] Consider replacing the manual index-based loop with Python’s reversed
for better readability. For example:
for feature_flag in reversed(feature_flags):
if feature_flag.get("id") == feature_flag_name:
return FeatureFlag.convert_from_json(feature_flag)
index = len(feature_flags) - 1 | |
while index >= 0: | |
if feature_flags[index].get("id") == feature_flag_name: | |
return FeatureFlag.convert_from_json(feature_flags[index]) | |
index -= 1 | |
for feature_flag in reversed(feature_flags): | |
if feature_flag.get("id") == feature_flag_name: | |
return FeatureFlag.convert_from_json(feature_flag) |
Copilot uses AI. Check for mistakes.
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.
I'm not sure if it maters given the data sizes of these. But going through backwards with the index will always be faster than calling reversed
every time, then looking backwards. Thoughts?
Co-authored-by: Copilot <[email protected]>
…cation Co-authored-by: zhiyuanliang-ms <[email protected]>
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.
Looks good to me now, but needs another approval as this is basically my change.
This PR implements the "last feature flag wins" behavior when multiple feature flags have the same ID in the configuration, aligning with the Microsoft Feature Management schema used in .NET and JavaScript implementations.
Problem
Previously, when multiple feature flags had the same
id
in the configuration, the first one encountered would be used. This behavior was inconsistent with other Microsoft Feature Management implementations where the last matching feature flag should win.Solution
Modified the feature flag resolution logic to:
Changes Made
Core Implementation
_get_feature_flag()
: Modified to iterate through all feature flags and return the last matching one_list_feature_flag_names()
: Updated to return only unique feature flag names while preserving orderExample Behavior
Testing
References
Fixes #52.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.