Skip to content
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

Support multiple cohorts in local evaluation #34

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

zaynetro
Copy link
Contributor

@zaynetro zaynetro commented Feb 14, 2024

  1. There's more than one cohort in the feature flag definition.

This restriction is annoying. Copying what NodeJS SDK does.

Ref: https://posthog.com/docs/feature-flags/local-evaluation

PS Sorry I couldn't reopen my old PR somehow :/

> 3. There's more than one cohort in the feature flag definition.

This restriction is annoying. And it seems it could be resolved rather
easily. Copying what NodeJS SDK does.

Ref: https://posthog.com/docs/feature-flags/local-evaluation
@neilkakkar
Copy link
Contributor

This is excellent, thanks @zaynetro , will review shortly

@neilkakkar neilkakkar self-requested a review February 15, 2024 11:06

require (
github.com/google/uuid v1.3.0
github.com/urfave/cli v1.22.5
)

require (
github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

these are a bit sus, what are they used for? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran go mod tidy. This seems to be a dependency of github.com/urfave/cli.

@zaynetro
Copy link
Contributor Author

Hi! Do you plan on merging this soonish? Is there something I can help with?

@neilkakkar
Copy link
Contributor

Sorry was off sick with a bad cold, didn't write a proper note on the approval. I'm just going to test this version with cohorts with a real project, and merge when all still looks good!

Thanks again for doing this :)

@neilkakkar
Copy link
Contributor

All good here :) - I found a separate issue where non-integer rollout percentages bork unmarshalling, but nothing related to this 👍

@neilkakkar neilkakkar merged commit 4944045 into PostHog:master Feb 21, 2024
1 check passed
@zaynetro zaynetro deleted the support-cohorts branch February 21, 2024 14:02
@neilkakkar
Copy link
Contributor

Thanks @zaynetro for all your recent contributions. I'd love to send you a posthog merch code, could you send me an email at neil @ posthog . com ?

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.

2 participants