-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: Add datafile accessor #240
Conversation
@@ -77,6 +77,11 @@ public enum OPTLYSDKVersion | |||
/// </summary> | |||
public bool? BotFiltering { get; set; } | |||
|
|||
/// <summary> |
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.
header update? 2020
/// <summary> | ||
/// Raw datafile | ||
/// </summary> | ||
public string Datafile { get; set; } |
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.
use private set
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.
abstract properties can not have private accessors.
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.
please address comments
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
@@ -14,6 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
using Newtonsoft.Json; |
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.
This doesn't seem to be used
///Returns the datafile corresponding to ProjectConfig | ||
/// </summary> | ||
/// <returns>the datafile string corresponding to ProjectConfig</returns> | ||
public string ToDatafile() |
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.
This isn't used. Any reason to keep it?
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/jobs/377668178