experiment overrides #1195
experiment overrides #1195
Conversation
There was a problem hiding this comment.
Thanks for this @SabrinaTardio! It's looking great, and I'm excited to move on to testing and reviewing platform PRs :) I'm dropping a bunch of naming comments, but feel free to push back as needed!
| @@ -20,6 +20,11 @@ import Combine | |||
| import Foundation | |||
| import Persistence | |||
|
|
|||
| /// A protocol for retrieving the current experiment cohort for feature flags if one has already been assigned. | |||
| protocol CurrentExperimentCohortProvider { | |||
There was a problem hiding this comment.
Let's rename it as CurrentExperimentCohortProviding, and I think it should be moved to FeatureFlagger.swift, below FlagCohort.
| @@ -18,7 +18,16 @@ | |||
|
|
|||
| import Foundation | |||
|
|
|||
| public protocol FlagCohort: RawRepresentable, CaseIterable where RawValue == CohortID {} | |||
| public protocol FlagCohort: CaseIterable, RawRepresentable where RawValue == CohortID {} | |||
There was a problem hiding this comment.
Let's rename it as FeatureFlagCohortDescribing and ideally move it below FeatureFlagDescribing. It would also benefit from a bit of documentation.
There was a problem hiding this comment.
I think I prefer the simpler version… also this was already the result of discussion and agreements
There was a problem hiding this comment.
Then I see two issues with it:
- it doesn't follow protocol naming that should use a gerund.
- for something that's dependent on
FeatureFlagDescribing, it doesn't follow the naming so we should rename the latter too.
| func getCohortIfEnabled<Flag: FeatureFlagExperimentDescribing>(for featureFlag: Flag) -> (any FlagCohort)? | ||
| /// > Note: Setting `allowOverride` to `false` skips checking local overrides. This can be used | ||
| /// when the non-overridden feature flag value is required. | ||
| func getCohortIfEnabled<Flag: FeatureFlagDescribing>(for featureFlag: Flag, allowOverride: Bool) -> (any FlagCohort)? |
There was a problem hiding this comment.
This function name looks too verbose, I'd say that cohort(for featureFlag:, allowOverride:) would work fine. Alternatives: activeCohort, effectiveCohort (effective because it's a combination of assigned + overridden, similar to NSAppearance.effectiveAppearance).
There was a problem hiding this comment.
also on this I am not too keen in changing it since it was the result of different discussions.
We can also revisit some naming in the future as it evolves.
| @@ -304,8 +306,8 @@ public class DefaultFeatureFlagger: FeatureFlagger { | |||
| case .remoteReleasable(let featureType), | |||
| .remoteDevelopment(let featureType) where internalUserDecider.isInternalUser: | |||
| if case .subfeature(let subfeature) = featureType { | |||
| if let resolvedCohortID = getCohortIfEnabled(subfeature) { | |||
| return Flag.CohortType.allCases.first { return $0.rawValue == resolvedCohortID } | |||
| if let resolvedCohortID = getCohortIfEnabled(subfeature, allowCohortReassignment: allowCohortAssignment) { | |||
There was a problem hiding this comment.
Can we stick to either assignment or reassignment?
| @@ -20,6 +20,11 @@ import Combine | |||
| import Foundation | |||
| import Persistence | |||
|
|
|||
| /// A protocol for retrieving the current experiment cohort for feature flags if one has already been assigned. | |||
| protocol CurrentExperimentCohortProvider { | |||
| func getCurrentCohortIfAssigned<Flag: FeatureFlagDescribing>(for featureFlag: Flag) -> (any FlagCohort)? | |||
There was a problem hiding this comment.
would assignedCohort(for featureFlag:) work well?
Just for transparency, I’ll address things/ask others opinion on major API changes on Monday |
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/1204186595873227/1209266907041599/f
iOS PR: duckduckgo/iOS#3892
macOS PR: duckduckgo/macos-browser#3798
What kind of version bump will this require?: Major/Minor/Patch
Optional:
Tech Design URL: https://app.asana.com/0/1204186595873227/1209266907041602/f
Description:
Steps to test this PR:
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template