• Anders429@lemmy.world
    link
    fedilink
    English
    arrow-up
    13
    ·
    1 year ago

    We need clearer rules on which pull requests require code reviews.

    Weird to me that any pull request would not require a code review.

    • Aloso@programming.dev
      link
      fedilink
      English
      arrow-up
      4
      ·
      1 year ago

      True, code for critical IT infrastructure should always be reviewed. But from what I understand, this is difficult because there is one full-time developer (paid by the Rust Foundation) and a small number of volunteers, who don’t have the time to review all the employee’s changes.

      • sugar_in_your_tea
        link
        fedilink
        English
        arrow-up
        1
        ·
        1 year ago

        Easy solution, give review rights to a few volunteers. Pick from the regular contributors.

        • Aloso@programming.dev
          link
          fedilink
          arrow-up
          1
          ·
          1 year ago

          On GitHub, everybody has the ability to review pull requests, even you. But there still aren’t enough volunteers who review PRs.

          • sugar_in_your_tea
            link
            fedilink
            arrow-up
            1
            ·
            1 year ago

            Sure, but you should always have a core contributor required to review code before it gets merged. That’s a feature GitHub offers, and it should be used. Block all PRs unless there’s at least one review from a trusted contributor, and consider requiring a second review from any source.

            • Aloso@programming.dev
              link
              fedilink
              arrow-up
              1
              ·
              1 year ago

              That doesn’t solve the issue that there are too few contributors. Requiring a review doesn’t ensure that someone reviews the code.

              • sugar_in_your_tea
                link
                fedilink
                arrow-up
                1
                ·
                1 year ago

                Requiring a review from a trusted contributor ensures that one of those trusted contributors reviews the code. The one main maintainer should add more people to that trusted circle, which will ensure that at least one of those will review all code that goes into the codebase.

                If people see that code isn’t being merged, someone will step up to request to be in that trusted circle.

    • CoderKat@lemm.ee
      link
      fedilink
      English
      arrow-up
      2
      ·
      edit-2
      1 year ago

      Yeah. At my current and previous jobs, literally everything going into an actual product required a code review, and that’s despite all the code being written by employees that you could generally trust. Even if my boss or literally the most experienced and trusted dev wrote a commit, it still needed a review.

      It’s feels weird submitting my own code without a review for side projects. So many bugs have been caught by reviewers that writing code that another person would use without it being reviewed feels just wrong.