posthog icon indicating copy to clipboard operation
posthog copied to clipboard

chore(flags): More refactorings to prepare for flag dependencies

Open haacked opened this issue 6 months ago • 1 comments

[!IMPORTANT] 👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Problem

Refactored the code to make it easier to understand and change. This is in preparation for adding flags that depend on other flags.

Related to #29016

Changes

Most of this PR is a set of extract method refactorings. I'll point them out to save time in review. I also moved all the flag_matching.rs tests into test_flag_matching.rs.

The big change is in graph_utils.rs and cohort_operations.rs where I refactored the graph to be a graph of objects (aka cohorts) rather than Ids. The main reason to do this is two-fold.

  1. It encapsulates the building and traversal of the graph so the caller doesn't have to understand it. All you have to do is build the graph and then call for_each_dependencies_first and add a callback to do what you want to each node.
  2. In the old code, we built the graph on IDs and then sorted the IDs. Then while traversing the IDs, we needed to iterate the list of cohorts to find the cohort with the ID every iteration. In most cases, this is fine. But a larger chain of cohorts could be a problem. But since we already know which cohort should be there, might as well build a graph of the cohorts.

There should be no observable behavioral changes with this PR.

Did you write or update any docs for this change?

How did you test this code?

Unit tests.

haacked avatar Jun 12 '25 23:06 haacked

Ugh, test_flag_matching.rs is so huge. I wanted to refactor it to use sub mods to group tests so they were a bit more organized (like I did with test_graph_utils.rs, but I really think we may want separate files to group these tests. For now I'll hold off.

haacked avatar Jun 13 '25 03:06 haacked