-
Notifications
You must be signed in to change notification settings - Fork 269
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 the entry point for Remote Config #478
Conversation
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 pretty good. Just one suggestion to simplify the implementation a bit.
private final FirebaseApp app; | ||
private final Supplier<? extends FirebaseRemoteConfigClient> remoteConfigClient; | ||
|
||
private FirebaseRemoteConfig(Builder builder) { |
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.
A builder is overkill for just 2 arguments. I'd recommend something simpler and direct:
@VisibleForTesting
FirebaseRemoteConfig(FirebaseApp app, FirebaseRemoteConfigClient client) {
this.app = checkNotNull(app);
this.client = checkNotNull(client);
}
private FirebaseRemoteConfig(FirebaseApp app) {
this(app, FirebaseRemoteConfigClientImpl.fromApp(app));
}
Note that I'm also not using a Supplier here. Since the client is needed for all operations in this API, there's no reason to lazy initialize it.
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.
Good point! Updated the code.
I had to update the unit test for testRemoteConfigClientWithoutProjectId()
. As the client is now initialized at construction it throws at FirebaseRemoteConfig.getInstance()
.
import org.junit.Test; | ||
|
||
public class FirebaseRemoteConfigTest { | ||
private static final FirebaseOptions TEST_OPTIONS = FirebaseOptions.builder() |
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.
Add space before
@hiranya911 |
@egilmorez : Please review the docs in |
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 with a nit.
|
||
static Builder builder() { | ||
return new Builder(); | ||
return new FirebaseRemoteConfig(app); |
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.
Remove fromApp
. Call constructor directly where it's needed.
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.
Thanks! Updated!
/** | ||
* This class is the entry point for all server-side Firebase Remote Config actions. | ||
* | ||
* <p>You can get an instance of FirebaseRemoteConfig via {@link #getInstance(FirebaseApp)}, and |
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.
Literal?
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.
Updated!
/** | ||
* Similar to {@link #getTemplate()} but performs the operation asynchronously. | ||
* | ||
* @return An {@code ApiFuture} that will complete with a {@link RemoteConfigTemplate} when |
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.
Suggest present tense, just "completes"
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.
Thanks! Updated!
src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfig.java
Show resolved
Hide resolved
6cf3a01
to
04906aa
Compare
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.
LG, thanks Lahiru!
FirebaseRemoteConfig.java
getTemplate()
andgetTemplateAsync()
Issue: #446
Note: Merging to
remote-config
branch.