Add Remote Config conditions to template #489
Conversation
lahirumaramba
force-pushed
the
lm-rc-conditions
branch
from
f0b1306 to
1a878a1
Compare
| * @param name A non-null, non-empty, and unique name of this condition. | ||
| * @param expression A non-null and non-empty expression of this condition. | ||
| */ | ||
| public Condition(@NonNull String name, @NonNull String expression) { |
There was a problem hiding this comment.
I think we need a constructor that accepts the color as well.
There was a problem hiding this comment.
Added a constructor with color. Should we amend the API proposal as well?
There was a problem hiding this comment.
Feel free to include a note. Looks like an oversight if it's not already there.
src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImplTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImplTest.java
Outdated
Show resolved
Hide resolved
| * @param name A non-null, non-empty, and unique name of this condition. | ||
| * @param expression A non-null and non-empty expression of this condition. | ||
| */ | ||
| public Condition(@NonNull String name, @NonNull String expression) { |
There was a problem hiding this comment.
Feel free to include a note. Looks like an oversight if it's not already there.
| * @param expression A non-null and non-empty expression of this condition. | ||
| * @param tagColor A non-null tag color of this condition. | ||
| */ | ||
| public Condition(@NonNull String name, @NonNull String expression, @NonNull TagColor tagColor) { |
There was a problem hiding this comment.
TagColor should be nullable.
src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImplTest.java
Show resolved
Hide resolved
lahirumaramba
force-pushed
the
lm-rc-conditions
branch
from
fc3ca65 to
128838e
Compare
lahirumaramba
force-pushed
the
lm-rc-conditions
branch
from
128838e to
6d2a377
Compare
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class ConditionTest { |
There was a problem hiding this comment.
I think this could be a good place to add more Condition class specific tests (constructors, getter, setters etc). But feel free to add them in future PRs.
There was a problem hiding this comment.
Good call. This makes me wonder... should we add test classes for all other types, too?
|
Hi @egilmorez! Please take a look at the docs when you get a chance. Thank you! |
| * | ||
| * @param name A non-null, non-empty, and unique name of this condition. | ||
| * @param expression A non-null and non-empty expression of this condition. | ||
| * @param tagColor A tag color of this condition. |
There was a problem hiding this comment.
I wonder if we might even want to clarify that this affects how the condition is displayed in the parameter list in the Firebase console. That's true, right? Outside the console, it is irrelevant?
There was a problem hiding this comment.
Ah kind of like you did for the setter below. Do you agree it would be good in both places? For creating and setting; not so much for getting I think.
-
The color associated with this condition for display purposes in the Firebase Console.
- Not specifying this value results in the console picking an arbitrary color to associate
- with the condition.
There was a problem hiding this comment.
Good call! I updated the docs here. Thanks!
Related Issue: #446