-
Notifications
You must be signed in to change notification settings - Fork 275
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 Version information to RC template #491
Conversation
3055c14
to
af11c4a
Compare
af11c4a
to
2f4ab8f
Compare
2f4ab8f
to
5123e8f
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.
It looks pretty solid. I only had some minor nits. But I do think we should support equals
on the new classes. A developer might want to do something like the following:
if (!template1.getVersion().equals(template2.getVersion()) {
// do something
}
It's also consistent with other classes in this package. WDYT?
src/test/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImplTest.java
Outdated
Show resolved
Hide resolved
if (!Strings.isNullOrEmpty(versionResponse.getUpdateTime())) { | ||
// Update Time is a timestamp in RFC3339 UTC "Zulu" format, accurate to nanoseconds. | ||
// example: "2014-10-02T15:01:23.045123456Z" | ||
// SimpleDateFormat cannot handle nanoseconds, therefore we drop nanoseconds from the string. |
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.
I noticed that the update time is represented in FC3339 UTC "Zulu" format, accurate to nanoseconds. Added new code here to convert the timestamp to milliseconds properly.
DateTime
does not handle nanoseconds so I had to drop them from the timestamp string before parsing. Otherwise SimpleDateFormat
considers nanoseconds as milliseconds and add it back to the time making the parsed date/time incorrect.
Apparently, there are better ways to handle nanoseconds in Java8 (LocalDateTime
and Instant
) :)
Implemented |
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. This looks great. Just a few nits.
f8bc871
to
a71dcd2
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.
LGTM with a couple of suggestions.
// SimpleDateFormat cannot handle nanoseconds, therefore we strip nanoseconds from the string. | ||
String updateTime = versionResponse.getUpdateTime(); | ||
int indexOfPeriod = !updateTime.contains(".") ? 0 : updateTime.indexOf("."); | ||
String updateTimeWithoutNanoseconds = updateTime.substring(0, indexOfPeriod); |
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.
indexOfPeriod can be possibly 0, in which case this will do substring(0, 0)
. I think following makes more sense to me:
String updateTime = versionResponse.getUpdateTime();
int indexOfPeriod = updateTime.indexOf('.');
if (indexOfPeriod != -1) {
updateTime = updateTime.substring(0, indexOfPeriod);
}
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.
I agree! This is better. Update the code. Thanks!
@@ -132,5 +132,12 @@ public void testEquality() { | |||
assertNotEquals(templateThree, templateFive); | |||
assertNotEquals(templateThree, templateSeven); | |||
assertNotEquals(templateFive, templateSeven); | |||
|
|||
final Template templateNine = new Template() |
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.
It's ok to define some of these as class level constants. For example templateNine can be a class-level constant, and templateTen can be initialized in the corresponding test case as a local var. Same goes for others.
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.
I see what you mean now! Moved to class level constants. Thanks!
Note: Since Version and User types are output only, I did not implementequal()
andhashcode()
operations. Instead, unit tests are checking for each property inVersion
andUser
individually. I also removed the equality operations fromTemplate
, as well.Version
andUser
) to keep consistency.Related to: #446