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

Add custom signals support in Remote Config. #13976

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

Conversation

tusharkhandelwal8
Copy link

@tusharkhandelwal8 tusharkhandelwal8 commented Oct 27, 2024

feat(rc): Add method to set custom signals for Remote Config Custom targeting

@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

Copy link
Contributor

github-actions bot commented Oct 27, 2024

Apple API Diff Report

Commit: fcbab6f
Last updated: Mon Dec 9 12:25 PST 2024
View workflow logs & download artifacts


FirebaseRemoteConfig

Enumerations

[ADDED] FIRRemoteConfigCustomSignalsError
[ADDED] FIRRemoteConfigCustomSignalsError
Swift:
+  typealias RemoteConfigCustomSignalsError . Code . _ErrorType = RemoteConfigCustomSignalsError
+    case unknown = 8101
+    case invalidValueType = 8102
+    case limitExceeded = 8103
Objective-C:
+  enum FIRRemoteConfigCustomSignalsError : NSInteger {}
+    FIRRemoteConfigCustomSignalsErrorUnknown = 8101
+    FIRRemoteConfigCustomSignalsErrorInvalidValueType = 8102
+    FIRRemoteConfigCustomSignalsErrorLimitExceeded = 8103

Classes

FIRRemoteConfig
[ADDED] -setCustomSignals:withCompletion:
Objective-C:
+  - ( void ) setCustomSignals :( nonnull NSDictionary < NSString * , NSObject *> * ) customSignals withCompletion :( void ( ^ _Nullable )( NSError * _Nullable error )) completionHandler NS_REFINED_FOR_SWIFT ;

Constants

[ADDED] FIRRemoteConfigCustomSignalsErrorDomain
[ADDED] FIRRemoteConfigCustomSignalsErrorDomain
Swift:
+  let RemoteConfigCustomSignalsErrorDomain : String
Objective-C:
+  extern NS_SWIFT_NAME ( RemoteConfigCustomSignalsErrorDomain ) NSString * const FIRRemoteConfigCustomSignalsErrorDomain

@tusharkhandelwal8 tusharkhandelwal8 marked this pull request as ready for review November 7, 2024 11:19
@tusharkhandelwal8 tusharkhandelwal8 changed the title Nc/custom signal Add custom signals support in Remote Config. Dec 5, 2024
@ncooke3 ncooke3 self-requested a review December 5, 2024 23:59
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

@tusharkhandelwal8 Thanks for working through all the feedback. LGTM!

Comment on lines +88 to +91
/// Sets custom signals for this Remote Config instance.
/// - Parameter customSignals: A dictionary mapping string keys to custom
/// signals to be set for the app instance.
func setCustomSignals(_ customSignals: [String: CustomSignalValue?]) async throws {
Copy link
Member

@ncooke3 ncooke3 Dec 13, 2024

Choose a reason for hiding this comment

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

Taking another pass here. I would recommend including a discussion point to the doc comment on the behavior that clients should expect when adding vs. inserting vs. removing.

Suggested change
/// Sets custom signals for this Remote Config instance.
/// - Parameter customSignals: A dictionary mapping string keys to custom
/// signals to be set for the app instance.
func setCustomSignals(_ customSignals: [String: CustomSignalValue?]) async throws {
/// Sets custom signals for this Remote Config instance.
/// - Parameter customSignals: A dictionary mapping string keys to custom
/// signals to be set for the app instance.
///
/// <insert a few sentences on adding/updating signals and using nil to delete a signal>
func setCustomSignals(_ customSignals: [String: CustomSignalValue?]) async throws {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants