Skip to content
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

fix(track): Send decisions for all experiments using an event when using track #136

Merged
merged 6 commits into from
Aug 21, 2018

Conversation

aliabbasrizvi
Copy link
Contributor

No description provided.

@aliabbasrizvi
Copy link
Contributor Author

build-e2e

@coveralls
Copy link

coveralls commented Aug 21, 2018

Coverage Status

Coverage increased (+0.0003%) to 99.654% when pulling 088d9d4 on ali/fix_conversion_python_sdk into 210e9c6 on master.

@aliabbasrizvi
Copy link
Contributor Author

This includes commits from #135


return snapshot
snapshot.setdefault(self.EventParams.EVENTS, []).append(event_dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to setdefault as opposed to just initializing an empty array with that event_dict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let me update that.

@@ -375,5 +375,152 @@ def setUp(self):
}]
}

self.optimizely = optimizely.Optimizely(json.dumps(self.config_dict))
self.config_dict_with_multiple_experiments = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be passed in for that one unit test that uses it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is ok

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

I left some comments, mostly nits

@aliabbasrizvi
Copy link
Contributor Author

build-e2e

@mikeproeng37
Copy link
Contributor

build-e2e

@aliabbasrizvi aliabbasrizvi merged commit a85e7ba into master Aug 21, 2018
@aliabbasrizvi aliabbasrizvi deleted the ali/fix_conversion_python_sdk branch August 21, 2018 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants