-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Add datafile accessor #392
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
Conversation
Pull Request Test Coverage Report for Build 1500
💛 - Coveralls |
unit test missing. toDatafile |
@@ -436,6 +443,7 @@ public Audience getAudience(String audienceId) { | |||
public String toString() { | |||
return "ProjectConfig{" + | |||
"accountId='" + accountId + '\'' + | |||
", datafile='" + datafile + '\'' + |
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.
Don't add it in toString
.
@mikeng13 @mjc1283 what do you suggest?
removed datafile from to 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.
A change requested
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfig.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/config/DatafileProjectConfig.java
Outdated
Show resolved
Hide resolved
core-api/src/main/java/com/optimizely/ab/optimizelyconfig/OptimizelyConfig.java
Outdated
Show resolved
Hide resolved
@@ -27,8 +26,6 @@ | |||
private Map<String, OptimizelyExperiment> experimentsMap; | |||
private Map<String, OptimizelyFeature> featuresMap; | |||
private String revision; | |||
|
|||
@JsonIgnore |
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 meant to remove @JsonIgnore redundant below at "getDatafile()".
Is it ok to remove @JsonIgnore for datafile completely?
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
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
Summary
Testing
https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/181371500