Skip to content

Add toJSON and fromJSON to Remote Config Template #500

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 10, 2020
Merged

Add toJSON and fromJSON to Remote Config Template #500

merged 10 commits into from
Dec 10, 2020

Conversation

lahirumaramba
Copy link
Member

@lahirumaramba lahirumaramba commented Dec 7, 2020 •
edited
Loading

  • Add toJSON() and fromJSON() to Template
  • Add unit tests

Related to: #446

@lahirumaramba lahirumaramba removed the request for review from hiranya911 December 7, 2020 22:51
hiranya911
hiranya911 suggested changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Haven't gone through everything. But I've added some high-level feedback around how the serialization logic can be improved.

hiranya911
hiranya911 suggested changes Dec 8, 2020
View reviewed changes
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. Looks mostly good. Few comments on cleaning up the PR a bit.

hiranya911
hiranya911 suggested changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Just a few minor nits.

hiranya911
hiranya911 reviewed Dec 9, 2020
View reviewed changes
src/test/java/com/google/firebase/remoteconfig/TemplateTest.java Outdated
.setVersion(expectedVersion);
Template template = Template.fromJSON(originalTemplate.toJSON());

assertEquals("etag-0010201", template.getETag());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not run assertEquals on the template instances?

Copy link
Member Author

Choose a reason for hiding this comment

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

Condition arrays are not quite the same for a direct equality check. The original template contains a condition that does not have a tag color. After the serialization -> deserialization process that tag color will be set to TagColor.UNSPECIFIED.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we fix that so the behavior remains consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should. Updated the code not to serialize the tag color as TagColor.UNSPECIFIED when null or empty.

hiranya911
hiranya911 approved these changes Dec 10, 2020
View reviewed changes
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion.

src/main/java/com/google/firebase/remoteconfig/RemoteConfigUtil.java Outdated
if (indexOfPeriod != -1) {
dateString = dateString.substring(0, indexOfPeriod);
}
checkArgument(!Strings.isNullOrEmpty(dateString), "Date string must not be null or empty");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should happen before everything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good catch! Thanks!

@lahirumaramba
Copy link
Member Author

LGTM with a suggestion.

Thanks for the review!! Made the suggested updates.

@lahirumaramba lahirumaramba merged commit 09043e6 into remote-config Dec 10, 2020
@lahirumaramba lahirumaramba deleted the lm-rc-from-json branch December 10, 2020 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@hiranya911 hiranya911 hiranya911 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lahirumaramba lahirumaramba

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lahirumaramba @hiranya911