Better completion callback for activate #5646
Conversation
| /// Completion handler invoked by activate method upon completion. | ||
| /// @param changed Flag indicating whether or not the configuration changed. | ||
| /// @param error Error message on failure. Nil if activation was successful. | ||
| typedef void (^FIRRemoteConfigActivateChangeCompletion)(BOOL changed, NSError *_Nullable error) |
There was a problem hiding this comment.
can you illustrate a bit on what the advantage is. I'm guessing using the boolean as first parameter will improve checking in swift but I don't know what exactly it is.
There was a problem hiding this comment.
It gives feedback to the API client about whether or not the config changed without an error being thrown. See #3586 for more on the issues caused by the other API.
There was a problem hiding this comment.
Nice, so this will align with Android.
805c9d0 to
eda90ad
Compare
| XCTAssertEqual(status, RemoteConfigFetchStatus.success) | ||
| self.config.activate { changed, error in | ||
| XCTAssertNil(error) | ||
| XCTAssert(changed) |
There was a problem hiding this comment.
Tiny nit: Can you change this to XCTAssertTrue? I did a double take on it, I think explicitly saying True makes it more clear.
| @@ -59,6 +60,43 @@ class FakeConsoleTests: APITestBase { | |||
| waitForExpectations() | |||
| } | |||
|
|
|||
| // Test New API. | |||
| // Contrast with testUnchangedActivateWillFlag in APITests.swift. | |||
| func testChangedActivateWillNotFlag() { | |||
There was a problem hiding this comment.
Should this also test the false, nil case where activate is called but no changes happened?
There was a problem hiding this comment.
Yes, but that test doesn't need the FakeConsole and was added to APITests.swift
3b6e88b to
58387fa
Compare
Adds an activate API with a changed parameter in the callback. It replaces a deprecated API that set an internal error if there was no change.
Fixes #3586
The integration test infrastructure is implemented in #5633
Googlers: See accompanying API proposal.