Skip to content

Conversation

@daibhin
Copy link
Contributor

@daibhin daibhin commented Jun 10, 2025

Follow on from #254

That PR had the intended effect of replacing import posthog with import posthoganalytics. I failed to realise that everywhere posthog.{method_name} was called also needed replacing. Going to try this simpler approach of never importing the entire package

@daibhin daibhin requested review from a team, hpouillot, oliverb123 and pauldambra June 10, 2025 09:44
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Updates import statements throughout the codebase to use specific function imports instead of the global posthog package import, improving maintainability and preventing circular dependencies.

  • Modifies posthog/sentry/posthog_integration.py to import specific functions (capture, host) instead of the global posthog package
  • Updates posthog/scopes.py to directly import capture_exception rather than accessing it through posthog.capture_exception
  • Removes sed commands from Makefile that were previously handling posthog/posthoganalytics import swapping
  • Bumps version to 4.6.2 to reflect the import statement cleanup

5 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile

Comment on lines 22 to 25
rm -rf posthog
python setup_analytics.py sdist bdist_wheel
twine upload dist/*
mkdir posthog
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Removing and recreating 'posthog' directory between operations could lead to issues if twine upload fails. Consider moving directory operations after successful upload.

raise ValueError("Something went wrong")
"""
import posthog
Copy link
Member

Choose a reason for hiding this comment

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

would be cool if we can have ruff forbid this in future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. ChatGPT is down right now and I'm having trouble googling for the right rule. Will double back

Copy link
Member

Choose a reason for hiding this comment

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

oh.. @daibhin i updated setup instructions in the readme
you need to run a command to setup the precommit hooks and then ruff will autorun on commit for you

@daibhin daibhin merged commit 4426dd9 into master Jun 10, 2025
6 checks passed
@daibhin daibhin deleted the dn-fix/correct-import branch June 10, 2025 10:05
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