• DontTakeMySky@lemmy.world
    link
    fedilink
    arrow-up
    9
    ·
    11 months ago

    Don’t just summarize the content though, summarize the rationale or how things connect. I can read your diff myself to see what changed, I want to know the logical connections, the reason you did X and not Y, etc.

    Or just say “stuff” and provide that context in the PR description separately, no need to overdo the commit log on a feature branch if you’re using squash merges from your PR.

    • deadbeef79000@lemmy.nz
      link
      fedilink
      arrow-up
      6
      ·
      edit-2
      11 months ago

      P1000x this.

      I can read a diff.

      I need to know why.

      No, a code comment isn’t good enough, it’s out of date after the next commit.

      • DontTakeMySky@lemmy.world
        link
        fedilink
        arrow-up
        1
        ·
        11 months ago

        Code comments for "why"s that persist. Commits for why’s that are temporary.

        If you need to run X before Y, add a comment. If you added X before why because it was easier, leave it in a commit

          • DontTakeMySky@lemmy.world
            link
            fedilink
            arrow-up
            2
            ·
            11 months ago

            With a comment on the test detailing why it matters so people don’t just assume the test is out of date when it fails.

            And ideally test the underlying result of x before y, not the fact that x is called before y.

            And while we’re at it, assert in Y that X has been called, and again comment the reason for the preconditions.