Review Please - eviltoast
  • dan@upvote.au
    link
    fedilink
    arrow-up
    1
    ·
    edit-2
    8 months ago

    Making PRs against a feature branch still has the same problem.

    Say you’re working on a major new feature, like adding a new log in flow. It’s a good idea to split it into many small changes: create initial log in form, implement user sessions, add SSO support, add “remember me” functionality, etc.

    Those changes will likely depend on each other, for example adding the “remember me” checkbox requires the form to actually be built, and you probably don’t want to wait for the reviewers to review one change before continuing your work. This means you naturally end up with PRs that depend on each other in a chain.

    Stacked PRs (or stacked diffs, stacked MRs, whatever your company calls it) means that:

    1. The code review tool lets you specify dependencies between the PRs, for example the “remember me” PR depends on the initial login form implementation
    2. It shows the dependencies visually in the UI, like a chain or tree
    3. Changes can’t be landed until the PRs they depend on have been reviewed
    4. There’s a way to land an entire stack
    5. When you review a PR later in the stack, it doesn’t show any of the changes that were made earlier in the stack. Each PR focuses just on the changes in that part.
    6. For each PR, the build steps and linters run for all the changes in the stack up until that point. It helps detect if incompatible changes are made earlier in the stack.

    Making all your commits directly to a branch then creating a PR for the whole branch is similar, but reviews are a nightmare since it’s only a single review for the entire branch, which can be thousands of lines of code.

    At my workplace, we use feature flags (essentially if statements that can be toggled on or off) rather than feature branches. Everyone works directly from the main branch. We use continuous deployment which means landed code is quickly pushed to production. New features are hidden behind a feature flag until they’re ready to roll out.

    • iknowitwheniseeit@lemmynsfw.com
      link
      fedilink
      arrow-up
      1
      ·
      8 months ago

      You can make a PR against your feature branch and have that reviewed. Then the final PR against your man branch is indeed huge, but all the changes have already been reviewed, so it’s just LGTM and merge that bad boy!

      • dan@upvote.au
        link
        fedilink
        arrow-up
        1
        ·
        8 months ago

        You can make a PR against your feature branch and have that reviewed

        But what if you have multiple PRs that depend on each other? Or are you saying only have one PR open at a time? That sounds like it’d be very slow?

        • iknowitwheniseeit@lemmynsfw.com
          link
          fedilink
          arrow-up
          1
          ·
          8 months ago

          I suppose it is possible to have two PR that have changes that depend on each other. In general this just requires refactoring… typically making a third PR removing the circular dependency.

          It sounds like your policy is to keep PR around a long time, maybe? Generally we try to have ours merged within a few days, before bitrot sets in.

          • dan@upvote.au
            link
            fedilink
            arrow-up
            1
            ·
            8 months ago

            Sorry, my comment was unclear. I didn’t mean a circular dependency, just PRs that have a chain of dependencies (e.g. PR 100 that depends on 99, that depends on 98, that depends on 97)

            They’re usually not around for a long time, but there can be relatively large chains if someone is quickly adding new features.