-
Notifications
You must be signed in to change notification settings - Fork 83
[FSSDK-11526] parse holdout from datafile into project config #1074
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
Conversation
also add getHoldoutsForFlag() function
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
This PR adds support for parsing holdout configurations from the datafile into ProjectConfig
and provides a helper to retrieve holdouts per flag.
- Introduces
ExperimentCore
andHoldout
types and extendsExperiment
- Extends
ProjectConfig
with holdout-related fields and implementsparseHoldoutsConfig
- Adds
getHoldoutsForFlag
helper and corresponding tests; a feature toggle is added infeature_toggle.ts
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
lib/shared_types.ts | Extracted ExperimentCore , added Holdout type |
lib/project_config/project_config.ts | Added holdout fields, parseHoldoutsConfig , and helper |
lib/project_config/project_config.spec.ts | Added tests for holdout parsing and retrieval |
lib/index.browser.ts | Imported new featureToggle module |
lib/feature_toggle.ts | Defined holdout feature flag stub |
Comments suppressed due to low confidence (3)
lib/shared_types.ts:121
- This interface property is missing a trailing semicolon, which will cause a TypeScript syntax error. Add a semicolon after the closing brace.
}
lib/index.browser.ts:22
- [nitpick] The imported
featureToggle
module isn't used in this file. If holdout logic isn't needed here, consider removing this import to clean up unused code.
import * as featureToggle from './feature_toggle';
lib/project_config/project_config.spec.ts:35
- [nitpick] The
mock
import fromnode:test
isn't used in these tests. Removing unused imports can improve readability.
import { mock } from 'node:test';
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 great. One missing part is to cover "optional" includeFlags and excludeFlags.
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.
LGTM
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.
LGTM
Summary
Test plan
Issues