-
Notifications
You must be signed in to change notification settings - Fork 21
LogHog -> PostHog #41
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
LogHog -> PostHog #41
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.
There are a couple of changes needed in this PR before we merge it. I'm happy to do them in this PR rather than waiting for you @martosaur since I know you're fairly busy. I've pointed this to a new v2.0.0 feature branch so that I/we can develop against that branch safely.
@martosaur Can you make sure I have write access to the parent log-hog-invasion
(😆) branch?
config/integration.exs
Outdated
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.
Do we need this one? It seems like this should be covered by integration.example.exs
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.
Right! It should be gitignored 😅 luckily this is a public token
d512b8c
to
196c732
Compare
Co-authored-by: Rafael Audibert <[email protected]>
Co-authored-by: Rafael Audibert <[email protected]>
@rafaeelaudibert that's a very short list so all done! But feel free to poke around more
I added you as a contributor to my fork so I think that should do it? |
@martosaur Just FYI, I'm travelling the last couple of weeks and have a busy upcoming week. I'll try and get back to this by August 15th if all goes well :) |
And yes, I have access to your repo now, thank you :) |
config/integration.exs
Outdated
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.
@martosaur I'm still seeing this file here. Will merge on the v2 branch and then delete this
@martosaur Anything against me merging this so that we can keep merging the other PRs while I work on the convenience layer for feature flags? |
@rafaeelaudibert Not at all, go for it! |
I merged both open PRs in the fork into this one, so once it's merged there should be no pending changes to worry about |
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 is so so good! Will be merging this in, and then working on the feature flags layer sometime next week - busy with the Revenue Analytics beta launch this week!
It feels weird to create a PR that basically replaces everything, but here we are 🫠 The code is pretty much identical to martosaur/log_hog#18 The biggest key question I think is which big parts of the original should survive.
We probably want to have a temporary branch for that too, since there is a definitely a list of things that should be done before this goes through, if it does at all. For example:
Sender
, so that a single process wouldn't become a bottleneck. I think Sentry's event sender is a decent examplePostHog.Core
for example?)Anyways, I'm leaving this up to you @rafaeelaudibert to outline the path forward , I'll do what I can to help.