8000 feat: add datafile accessor by pthompson127 · Pull Request #275 · optimizely/python-sdk · GitHub
[go: up one dir, main page]
Skip to content

feat: add datafile accessor #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.

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 13 commits into from
Jul 1, 2020
Merged

feat: add datafile accessor #275

merged 13 commits into from
Jul 1, 2020

Conversation

pthompson127
Copy link
Contributor
@pthompson127 pthompson127 commented Jun 18, 2020 •
edited
Loading

Summary

  • Add datafile accessor methods/functionalities

Testing

  • Ran manual tests

@pthompson127
Copy link
Contributor Author
pthompson127 commented Jun 18, 2020 •
edited
Loading

Wanted to get thoughts on this implementation before proceeding (especially on optimizely_config.py). Constructor was changed due to the existence of OptimizelyConfigService and to enable OptimizelyConfig to get the datafile and match the same user interface described in Option 2.

I realize that this will change the format that OptimizelyConfig currently adheres to but I don't really see how to avoid this.

@coveralls
Copy link
coveralls commented Jun 22, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 97.099% when pulling f12dd62 on peter/get-datafile into 1bf5a89 on master.

@pthompson127 pthompson127 marked this pull request as ready for review June 23, 2020 21:48
@pthompson127 pthompson127 requested a review from a team as a code owner June 23, 2020 21:48
@pthompson127 pthompson127 removed their assignment Jun 23, 2020
mjc1283
mjc1283 approved these changes Jun 24, 2020
View reviewed changes
Copy link
@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

LGTM. Please await @aliabbasrizvi's review as well.

aliabbasrizvi
aliabbasrizvi reviewed Jun 24, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi reviewed Jun 24, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi reviewed Jun 24, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi reviewed Jun 24, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi suggested changes Jun 24, 2020
View reviewed changes
Copy link
Contributor
@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Small changes.

aliabbasrizvi
aliabbasrizvi reviewed Jun 26, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor
@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Looks good. Just a minor comment update.

@pthompson127 pthompson127 merged commit ddf06ed into master Jul 1, 2020
@pthompson127 pthompson127 deleted the peter/get-datafile branch July 1, 2020 17:48
oakbani added a commit that referenced this pull request Jul 6, 2020
@oakbani
Revert "feat: add datafile accessor (#275)"
1b03359 
This reverts commit ddf06ed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjc1283 mjc1283 mjc1283 approved these changes

@pawels-optimizely pawels-optimizely pawels-optimizely left review comments

@aliabbasrizvi aliabbasrizvi aliabbasrizvi approved these changes

Assignees

@pthompson127 pthompson127

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pthompson127 @coveralls @mjc1283 @aliabbasrizvi @pawels-optimizely @optimizely-codereview-bot
0