Skip to content

Comments

Add initial classes for Remote Config API#477

Merged
lahirumaramba merged 4 commits intoremote-configfrom
lm-rc-init
Sep 15, 2020
Merged

Add initial classes for Remote Config API#477
lahirumaramba merged 4 commits intoremote-configfrom
lm-rc-init

Conversation

@lahirumaramba
Copy link
Member

@lahirumaramba lahirumaramba commented Sep 10, 2020 •
edited
Loading

  • Add FirebaseRemoteConfigClient interface and implementation
  • Add basic classes for error handing
  • Add unit tests for FirebaseRemoteConfigClientImpl

Note: merging to remote-config branch.

Related issue: #446

@lahirumaramba lahirumaramba force-pushed the lm-rc-init branch 2 times, most recently from 498018e to 74c21d3 Compare September 10, 2020 20:53
@lahirumaramba lahirumaramba marked this pull request as draft September 10, 2020 21:24
@lahirumaramba lahirumaramba marked this pull request as ready for review September 10, 2020 21:56
hiranya911
hiranya911 suggested changes Sep 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.

Looks mostly good. I've added a series of questions and comments on things that we can clean up.

hiranya911
hiranya911 suggested changes Sep 11, 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.

It's getting there. Just needs a bit of cleanup around error handling.

src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImpl.java Outdated
"ETag header is not available in the server response.", null, null,
RemoteConfigErrorCode.INTERNAL);
}
checkArgument(!(etagList == null || etagList.isEmpty()),
Copy link
Contributor

@hiranya911 hiranya911 Sep 11, 2020

Choose a reason for hiding this comment

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

Perhaps checkState is more appropriate here.

src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImpl.java Outdated
"ETag header is not available in the server response.", null, null,
RemoteConfigErrorCode.INTERNAL);
}
checkArgument(!(etagList == null || etagList.isEmpty()),
Copy link
Contributor

@hiranya911 hiranya911 Sep 11, 2020

Choose a reason for hiding this comment

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

Also checkState(etagList != null && !etagList.isEmpty())

src/main/java/com/google/firebase/remoteconfig/FirebaseRemoteConfigClientImpl.java Outdated
"ETag header is not available in the server response.", null, null,
RemoteConfigErrorCode.INTERNAL);
}
checkArgument(!Strings.isNullOrEmpty(etag),
Copy link
Contributor

@hiranya911 hiranya911 Sep 11, 2020

Choose a reason for hiding this comment

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

Here too. I'd even recommend adding a getETag(IncomingResponse resp) helper method, and doing all the validation there.

hiranya911
hiranya911 approved these changes Sep 14, 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. LGTM 👍

src/main/java/com/google/firebase/remoteconfig/internal/RemoteConfigServiceErrorResponse.java Outdated
int separator = message.indexOf(':');
if (separator != -1) {
String errorCode = message.substring(0, separator).replaceAll("\\[|\\]", "");
Matcher errorMatcher = Pattern.compile("^\\[(\\w+)\\]:.*$").matcher(message);
Copy link
Contributor

@hiranya911 hiranya911 Sep 14, 2020

Choose a reason for hiding this comment

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

Pattern.compile() could be expensive. Pre-compute it and store as a static constant.

Copy link
Member Author

@lahirumaramba lahirumaramba Sep 15, 2020

Choose a reason for hiding this comment

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

Good point! Thanks!

@lahirumaramba lahirumaramba merged commit 88482db into remote-config Sep 15, 2020
@lahirumaramba lahirumaramba deleted the lm-rc-init branch September 15, 2020 20:51
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