feat(flags): Extract flag definitions from redis #42
Conversation
neilkakkar
changed the title
| pub enum OperatorType { | ||
| #[serde(rename = "exact")] | ||
| Exact, | ||
| #[serde(rename = "is_not")] |
There was a problem hiding this comment.
nit: you should be able to use a toplevel #[serde(rename_all = "snake_case")] here, see https://serde.rs/container-attrs.html
While annotating each field might feel "safer", I think there's a higher risk of errors than getting the machine to do it.
There was a problem hiding this comment.
oh, some of them bork here, but I guess I can still do it for the majority, thanks, will do!
| #[derive(Debug, Deserialize, Serialize)] | ||
| pub enum GroupTypeIndex {} | ||
|
|
||
| #[derive(Debug, Deserialize, Serialize)] |
There was a problem hiding this comment.
nit: do you need these types to be Serialize? It'll slightly increase compilation time, so it's better to only derive as needed
There was a problem hiding this comment.
Thanks, will fix! I was thinking I'd need it for tests, but then I'm building them all via json, so unnecessary 👍
There was a problem hiding this comment.
Thanks, addressed all your comments in the next PR :) - and added TODOs for things I'm deferring.
| let serialized_flags = client | ||
| .get(format!("{TEAM_FLAGS_CACHE_PREFIX}{}", team_id)) | ||
| .await | ||
| .map_err(|e| match e { |
There was a problem hiding this comment.
nit: you'll have to refactor this into an Into on the third code path ^^
| assert_eq!(flag.key, "flag1"); | ||
| assert_eq!(flag.team_id, team.id); | ||
| assert_eq!(flag.filters.groups.len(), 1); | ||
| assert_eq!(flag.filters.groups[0].properties.as_ref().unwrap().len(), 1); |
There was a problem hiding this comment.
nit: I prefer to use .expect("the hypothesis that proved false") instead of .unwrap(), to help with debugging.
I didn't enable the "don't allow unwrap lint" on, but I will someday
There was a problem hiding this comment.
aha, makes sense, will switch to this 👌
towards PostHog/posthog#22131
I'm getting into more business logic territory, so pipeline reviews optional, but I'll always appreciate them :love-hog: