local sixpack #6
Conversation
maurerbot
commented
Apr 20, 2015
maurerbot
commented
- Waiting on pypi release for sixpack-py
- Add bucket param to participate once Choose bucket sixpack/sixpack#170 is merged
- Merge once an up stream pull request to seatgeek for Sixpack-client==1.1.2 is merged
| except RequestException as e: | ||
| logger.exception("Error while trying to .convert: {err}".format(err=e)) | ||
| logger.exception("Error while trying to .convert: %s" % e) |
There was a problem hiding this comment.
Trying to comply with landscape. I'm going to revert and ignore.
There was a problem hiding this comment.
Interesting - reading the comment in landscape, I agree with it. What it's saying, though, is that you should do logger.exception("Error while trying to .convert: %s", e).
Both the original and this change unconditionally do the substitution, but doing it like I have here only does the substitution if the logging is actually needed, for a minor performance improvement.
correct issues prefetch not force fix test docs docs better doc
|
v1.1.1 of sixpack-py will be tagged Monday |
| Tracking Locally | ||
| ----------------- | ||
|
|
||
| By passing in the argument `local` in the SixpackTest constructor you can telldjango-sixpack to create a convertable db record for each participant. |
| @@ -5,6 +5,8 @@ | |||
| from django.conf import settings | |||
| from requests.exceptions import RequestException | |||
|
|
|||
| from .models import SixpackParticipant | |||
There was a problem hiding this comment.
As per "Writing Idiomatic Python", we prefer using absolute imports to relative imports.
| if self.local: | ||
| try: | ||
| participant = SixpackParticipant.objects.get(unique_attr=self.client_id, experiment_name=experiment_name) | ||
| participant.convert = True |
There was a problem hiding this comment.
Move lines 114 and 115 into an else.
| Tracking Locally | ||
| ----------------- | ||
|
|
||
| By passing in the argument `local` in the SixpackTest constructor you can tell django-sixpack to create a convertible db record for each participant. |
There was a problem hiding this comment.
Can you add docs about why you'd want to do this/use cases for this? I get that it's creating a record in the DB, but not why I'd like to do that.
|
|
||
| class SixpackParticipant(models.Model): | ||
| """ | ||
| A model that contains fields relating to a sixpack experiment participant. |
There was a problem hiding this comment.
Can you either make a more fulsome docstring (what do each field mean?), or drop this.
|
|
||
| try: | ||
| resp = session.participate(experiment_name, self.alternatives, force) | ||
| resp = session.participate(experiment_name, self.alternatives, force=force, prefetch=prefetch) | ||
| except RequestException: | ||
| logger.exception("Error while trying to .participate") |
There was a problem hiding this comment.
While you're in here, please make the quotation use consistent.
|
LGTM |
|
|
|
|
|
@dlanger @adrianmaurer - re: South - this could be a bit tricky - grab me first thing in the morning and we can go over it. The short of it is, you will need to make South==1.0.0 a requirement of this package. |
|
right, south isn't included by default in 1.5. |
|
|
|
|
|
|
|
|
||
| experiment = ButtonColorTest(local=True) | ||
|
|
||
| You may also choose to track only locally by passing in `sixpack=False` to the test constructor. |
There was a problem hiding this comment.
This description doesn't match the sample code on line 106.
|
|
|
|
|
|
|
Closing as stale. |
