Hey, I want your opinion on code reviews, what is the best way to use them in a professional environment? Pick one of the following and give me your thoughts (from the most forgiving to the most strict):

  1. no code reviews, they are useless
  2. optional code reviews
  3. mandatory reviews on code that is already merged, optional fixes
  4. mandatory reviews on code before merging (like a pull request), with a time-frame for optional fixes (i.e. whether to fix what has been pointed out is up to the author), merge will occur anyway.
  5. mandatory reviews on code before merging (PR) with mandatory fixes.

Of course in open source development with public contributions, you’ll often see (5), but I’m not convinced it could work in professional dev.

Edit: I’m talking about a team of 5 mid to senior devs (no junior or interns) working on a 2-3 year project without many security concerns, but feel free to give me your general opinion.

  • mattreb@feddit.itOP
    0·
    6 months ago

    Everything gets reviewed. If you have a constructive comment you put it in the review.

    I’m a tech lead now so that’s basically the end although if they want to argue their case I’ll hear them out.

    So basically you’re saying that all the comments are welcome, and should be argued, but you have to appoint a person that have the final say on what to merge right?

    I’m not sure what problem you think is being moved to the reviewer.

    The problem of deciding what should be merged or blocked, I’ve mainly made this post to understand that, so saying “#5 or you’re crazy” doesn’t answer much. But I’ve probably worded that badly.

    • MagicShel@lemmy.zipEnglish
      1·
      6 months ago

      If a story gets created, the code will be merged… when it’s right. If you’re talking OSS, then I am out of my element, but I’ll wager there’s no universal answer because each code owner sets their own standards.

      you have to appoint a person that have the final say on what to merge right?

      If there is work that needs to be done, and you are asked to do it, the code will be merged when it’s right. I don’t decide what to merge, I decide when something is ready to merge.

      The problem of deciding what should be merged or blocked

      If you want to merge something and I read it over and reject the PR because you forgot about concurrency, that doesn’t mean you don’t get to merge, it means that it’s not finished baking. And assuming you give a shit about the code your response should be “oh shit, lemme fix that and resubmit” OR “actually this code will never have concurrent access, and here’s why.”

      You’re making this process sound adversarial when it isn’t. It’s a group effort. Everyone wins or loses together.