• DieguiTux8623@feddit.it
    link
    fedilink
    arrow-up
    21
    ·
    1 year ago

    The worst case is when someone requires changes, you address them, but then they disappear/go on a leave.

    If the repository rules require all conversations to be resolved before merging and only the original reviewer can mark them as solved, the PR is stuck forever even if the rest of the team approves it.

  • pimeys@lemmy.nauk.io
    link
    fedilink
    arrow-up
    10
    ·
    1 year ago

    A 15000 line PR landing on a Friday evening for the lucky random reviewer to open on Monday. “Please approve it fast so we avoid too many conflicts.”

    • ohlaph@lemmy.world
      link
      fedilink
      arrow-up
      5
      ·
      edit-2
      1 year ago

      I would review it and immediately tell them to break it into bite sized PRs.

      My coworker kept doing that. We had several talks about it. Other members of our team had talks about it with them, and even our manager. Finally, I marked the PR as needs work, told them to break it into several PRs. They weren’t happy, but I was tired of dealing with PRs that were 30+ files, unrelated in change, and over 1500 lines of code changes. They were pretty mad at me for a while. But it stopped shortly afterwards.

      It shouldn’t take more than an hour to review a PR.

      • douglasg14b@programming.dev
        link
        fedilink
        arrow-up
        1
        ·
        edit-2
        1 year ago

        There is no context here though?

        If this is a breaking change to a major upgrade path, like a major base UI lib change, then it might not be possible to be broken down into pieces without tripping or quadrupling the work (which likely took a few folks all month to achieve already).

        I remember in a previous job migrating from Vue 1 to Vue 2. And upgrading to an entirely new UI library. It required partial code freezes, and we figured it had to be done in 1 big push. It was only 3 of us doing it while the rest of the team kept up on maintenance & feature work.

        The PR was something like 38k loc, of actual UI code, excluding package/lock files. It took the team an entire dedicated week and a half to review, piece by piece. We chewet through hundreds of comments during that time. It worked out really well, everyone was happy, the timelines where even met early.

        The same thing happened when migrating an asp.net .Net Framework 4.x codebase to .Net Core 3.1. we figured that bundling in major refactors during the process to get the biggest bang for our buck was the best move. It was some light like 18k loc. Which also worked out similarly well in the end .

        Things like this happen, not that infrequently depending on the org, and they work out just fine as long as you have a competent and well organized team who can maintain a course for more than a few weeks.

        • douglasg14b@programming.dev
          link
          fedilink
          arrow-up
          1
          ·
          edit-2
          1 year ago

          The follow on. Lots and LOTS of unrelated changes can be a symptom of an immature codebase/product, simply a new endeavor.

          If it’s a greenfield project, in order to move fast you don’t want to gold plate or over predictive future. This often means you run into misc design blockers constantly. Which often necessitate refactors & improvements along the way. Depending on the team this can be broken out into the refactor, then the feature, and reviewed back-to-back. This does have it’s downsides though, as the scope of the design may become obfuscated and may lead to ineffective code review.

          Ofc mature codebases don’t often suffer from the same issues, and most of the foundational problems are solved. And patterns have been well established.

          /ramble

        • ohlaph@lemmy.world
          link
          fedilink
          arrow-up
          1
          ·
          1 year ago

          It was simply constant refactors, moving random stuff, etc. like, every week. It was unnecessary change.

      • pimeys@lemmy.nauk.io
        link
        fedilink
        arrow-up
        1
        ·
        1 year ago

        Yeah I’ve been working a lot in my life in seed stage startups and it is quite common in the early stages… I try to make things change though.

  • Cheesebaron@feddit.dk
    link
    fedilink
    arrow-up
    6
    arrow-down
    12
    ·
    1 year ago

    This always happens to me when I’ve written some genius code. Takes so long to review it, because my caveman colleagues don’t understand it.

    • Cheesebaron@feddit.dk
      link
      fedilink
      arrow-up
      6
      arrow-down
      1
      ·
      1 year ago

      It is amazing how many cavemen didn’t understand the sarcasm of my comment, and in their rage had to downvote it to hell. 😂

      • sbv
        link
        fedilink
        English
        arrow-up
        4
        ·
        1 year ago

        I feel like we Lemmites are not great at humour and we are quick to downvote. 🤷

        Were you referencing https://grugbrain.dev/ ? I only ran into that recently, and I got a real kick out of it.

        • Cheesebaron@feddit.dk
          link
          fedilink
          arrow-up
          2
          arrow-down
          1
          ·
          1 year ago

          I read that recently as well, it is a great read. I can relate to a lot of the things. While it is meant as a humor piece, there is some solid advice in there.

          I didn’t exactly have it in mind when I wrote my comment, but maybe subconsciously 😅

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

            I’ve recently started dealing with architects. A lot of the comments hit close to home.