• Murdo Maclachlan@lemmy.world
    link
    fedilink
    English
    arrow-up
    3
    ·
    edit-2
    1 year ago

    Image Transcription: Twitter


    Giray Özil, @girayozil

    Ask a programmer to review 10 lines of code, he’ll find 10 issues. Ask him to do 500 lines and he’ll say it looks good.


    I am a human who transcribes posts to improve accessibility on Lemmy. Transcriptions help people who use screen readers or other assistive technology to use the site. For more information, see here.

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

    Y’all need to get yourselves some PR review automation in place. Stop wasting time on trivial reviews and requesting changes for common problems so that when you ping a colleague for a code review, they know it’s important rather than a simple request for a thumbs up.

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

    That’s why PR should be small. It’s much better to have multiple PRs than a single big one.

    Totally fair to have gigantic PR full of boilerplate code, but generally you can split the boilerplate and your feature in 2 PRs, where only the feature will get a proper review.

    All of this obviously depends on the criticality of the system :p

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

      That can lead to another problem though, which is that if a developer knows a merge is only part of the whole change, it becomes easy to assume any issues will be handled elsewhere.

  • Midas@ymmel.nl
    link
    fedilink
    arrow-up
    1
    ·
    1 year ago

    Really grinds my gears to get a review request for a huge PR with almost no context and no instructions on how to run the code / test data.

    I’ve even had one guy ask for a review who hadn’t even manually tested his own codes happy path. Sure he wrote some unit tests and those ran but once you actually tried using the code in the app all kinds of exceptions and weird situations came up. No idea how people dare do that shit.

  • Astiolo@lemmy.fmhy.ml
    link
    fedilink
    arrow-up
    1
    ·
    edit-2
    1 year ago

    This can be called the bike shed effect or Law of Triviality. It’s not just programming where a simple digestible idea will be contested because it’s easy to poke holes, while something more complicated and more consequential is much harder, so it gets little to no resistance.

  • bleistift2@feddit.de
    link
    fedilink
    arrow-up
    1
    ·
    1 year ago

    I know this implies that the reviewer didn’t care to read the bigger PR, but I think this might actually be legit. If your PR is only 10 lines long, then chances are those lines are very dense, or intricate in some other way. However, if you submit 500 lines, then it’s probably mostly boilerplate code with trivial adaptions.

    No-one in their right mind would submit 500 lines of substantial code.

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

      Has no one here ever worked on a new project or even a new feature in a decently sized codebase? Working exclusively in maintenance / minor change mode has to be exhausting.

      • bleistift2@feddit.de
        link
        fedilink
        arrow-up
        1
        ·
        edit-2
        1 year ago

        Depends on what you classify as “minor change”. When I took up my first professional project, I found a plethora of little things to improve which would make users happy. That was very satisfying.

        On the other hand, writing yet another module that displays a list of Foos, lets the user create a Foo, show the details of Foo, update it, and delete a Foo, becomes dull quickly, despite being a “new feature”.

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

    True, but the 10 line change can me merged two weeks from now and it won’t make any difference. If you let the 500 line merge languish for two days you’ll have screwed up the work of three other programmers.

  • Ser Salty@feddit.de
    link
    fedilink
    arrow-up
    1
    ·
    1 year ago

    If a programmer does 500 lines, how much work will they get done before their heart explodes?

    • TheGreenGolem@lemm.ee
      link
      fedilink
      arrow-up
      0
      ·
      1 year ago

      Why. Whyyyyyy people need to comment this always? Why isn’t just the Approve button enough? I so much hate it.

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

        Ah, that’s too boring. I have a range of responses to pick from to keep things interesting:

        • LGTM
        • Nice
        • Looks good
        • Thanks
        • Looks great
        • :thumbsup:
        • Looks good to me
        • :shipit:

        For me, no text means “I haven’t really reviewed this properly so don’t want to write anything that could be used against me if (when?) this breaks something in prod”

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

        In my experience, the managers get confused when issues/PRs are closed without any comment.
        Useless comments beat having them pop into your slack to ask “hey, did you review this?” with a link to an approved PR.

      • HorseWife@midwest.social
        link
        fedilink
        arrow-up
        0
        ·
        1 year ago

        If you’re in a place with codebase analytics you want to have at least one comment on every MR - otherwise the system will start to think you’re falling behind… I hate codebase analytics.