search

cooncesean / mixpanel-query-py

The Python interface to fetch data from Mixpanel.
MIT License
29 stars 17 forks source link

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

Closed robin900 closed 9 years ago

robin900 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

frifri commented 9 years ago

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

robin900 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

cooncesean commented 9 years ago

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