Skip to content

added six module for py3 compatibility; migrated all code#15

Merged
cooncesean merged 3 commits intocooncesean:masterfrom
robin900:master
Jun 22, 2015
Merged

added six module for py3 compatibility; migrated all code#15
cooncesean merged 3 commits intocooncesean:masterfrom
robin900:master

Conversation

@robin900
Copy link
Copy Markdown
Contributor

Tested with 2.7.6 and 3.4.3. Haven't tested every method, but request generation and response json parsing works fine so I foresee no problems.

@cooncesean
Copy link
Copy Markdown
Owner

Hey @robin900 - thx for taking the time to get this project up to speed and compatible with Python 3. I have some light feedback and if you don't mind addressing, I'll get this merged asap.

  1. Docstrings: Would you mind adding some docstrings for the new _totext() and _tobytes() functions?
  2. utils.py: Would you mind moving these two fcns to a utils.py module (mixpanel_query/utils.py)? I understand these fcns are currently only used in the modules in which they are declared, but this IMO, change would help keep the project organized as it grows.

Thx again for the work and the PR -- I appreciate your help :)

@cooncesean
Copy link
Copy Markdown
Owner

@thesmallestduck @frifri -- would you mind reviewing these changes when you get a minute?

@frifri
Copy link
Copy Markdown
Collaborator

frifri commented Jun 22, 2015

@robin900 Thanks for that PR.
other than @cooncesean feedback, this looks good to me.

@robin900
Copy link
Copy Markdown
Contributor Author

I'll push a commit with the changes you requested.

Would you want to create a live Mixpanel account, with whatever email address you prefer as the login, and seed some events in it, so that we can have a test suite that hits a live Mixpanel account? I have a tox.ini and a unittest-based test suite ready to go if you are willing to make an MXP account, put some events in, and add the _secret and _key to the test file. I'll make a separate PR for those.

@robin900
Copy link
Copy Markdown
Contributor Author

commit pushed. should be ready to merge. if you could make a PyPI release soon I can remove my git+ssh urls for mixpanel-query-py from all my projects :)

BTW, my next PR will have a tox.ini and a test suite (very basic). But the test suite will fail because it doesn't have a valid key/secret for a Mixpanel account for testing.

@cooncesean
Copy link
Copy Markdown
Owner

Huge - thx @robin900; we'll merge today and release to PyPi; I'll update this thread once that has been done.

As for tests, do you think we could get away with using mock for the suite -- or do you think the library is so dependent on Mixpanel integration that its necessary to hit their service and check for results. I'd hate to have a failing test suite b/c Mixpanel is slow or unresponsive. Hit back with any thoughts you have.

@thesmallestduck
Copy link
Copy Markdown
Collaborator

Haven't tested locally, but reads good. Thanks for doing this @robin900

cooncesean added a commit that referenced this pull request Jun 22, 2015
added six module for py3 compatibility; migrated all code
@cooncesean cooncesean merged commit 2ed343a into cooncesean:master Jun 22, 2015
@cooncesean
Copy link
Copy Markdown
Owner

@robin900 Release 0.1.6 is on PyPi -- can you pip install and assert it works as expected?

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.

4 participants