-
Notifications
You must be signed in to change notification settings - Fork 9
Fix deprecated Posthog API Key initialization #91
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's look up in which posthog package version this was applied, and put it here:
Line 18 in 3fdc748
"posthog", |
also, the docs are failing because we're missing this: https://github.com/ploomber/jupysql/blob/6f83dbec7659124efe00c8ab1c3838b404fc0f3f/.readthedocs.yml#L24
the CI is also failing, I think because the Python versions are too old. let's only test with 3.10, 3.11 and 3.12
setup.py
Outdated
@@ -15,7 +15,7 @@ | |||
|
|||
REQUIRES = [ | |||
"pyyaml", | |||
"posthog", | |||
"posthog<3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the pin should be the other way around: >x.y.z
your previous changes were fine but we need to pin to ensure that the new version also forces a posthog upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinned the version to >=3.0
@edublancas before I saw your review I went back and pinned the version instead of making the changes (since I saw the tests were failing due to PH not supporting python 3.8) If you're okay with removing the python 3.8 tests then I'll bring back the original changes and apply your notes |
im ok removing old python versions |
Tests are failing due to failed mocks. Updating them now |
@edublancas please review again when you get the chance |
self._posthog_client = posthog.Posthog( | ||
api_key, host="https://us.i.posthog.com" | ||
) | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't a good way to handle an incompatibilty, instead we can raise an error telling users they must upgrade posthog or downgrade ploomber-core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: ca6301a
Describe your changes
posthog.project_api_key = self.api_key
since its deprecatedposthog.Posthog(api_key, host='https://us.i.posthog.com')
and changed thelog_api
function to use this clientIssue number
Fixes support issue: https://ploomber-io.slack.com/archives/C08QNPDF15J/p1752779413170409
Checklist before requesting a review
pkgmt format
📚 Documentation preview 📚: https://ploomber-core--91.org.readthedocs.build/en/91/