Skip to content
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

feat: report prerequisite relations in AllFlagState #19

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cwaldren-ld
Copy link
Contributor

@cwaldren-ld cwaldren-ld commented Oct 22, 2024

This PR updates AllFlagsState to track prerequisite evaluations.

This didn't require modifying the Evaluator interface, as it already returns prerequisites as part of the EvalResult.

When the returned FlagState is marshaled to JSON, it will now contain the prerequisite info for each flag.

@@ -295,7 +332,10 @@ public override FeatureFlagsState Read(ref Utf8JsonReader reader, Type typeToCon
for (var flagsObj = RequireObject(ref reader); flagsObj.Next(ref reader);)
{
var subKey = flagsObj.Name;
var flag = flags.ContainsKey(subKey) ? flags[subKey] : new FlagState();
Copy link
Contributor Author

@cwaldren-ld cwaldren-ld Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly awkward: have to new the prerequisite list here in the deserializer, and also on line 375. I'd rather just have this in the constructor so it's never a question, but looks like structs in our language version can't define constructors.

Another option is to make a null list be a possibility. Logically that'd be fine, but.. I'd rather the consumers never need to question if it's null or not. I could be convinced otherwise.

@@ -371,8 +372,7 @@ public FeatureFlagsState AllFlagsState(Context context, params FlagsStateOption[

var builder = new FeatureFlagsStateBuilder(options);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These weren't used at all in this function. They are used internal to the FeatureFlagsStateBuilder, presumably they didn't get cleaned up when that was introduced.


var directPrerequisites = result.PrerequisiteEvals.Where(
e => e.FlagKey == flag.Key)
.Select(p => p.PrerequisiteFlag.Key).ToList();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting to a list here, rather than leaving as an IEnumerable, because the intent is that order is preserved.

@cwaldren-ld cwaldren-ld marked this pull request as ready for review October 22, 2024 22:02
@cwaldren-ld cwaldren-ld requested a review from a team as a code owner October 22, 2024 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant