983
Peer reviewing is torture now (europe.pub)
submitted 3 weeks ago by to c/programmer_humor@programming.dev
137 comments fedilink hide all child comments
 
you are viewing a single comment's thread
view the rest of the comments
[–] 2 points 2 weeks ago* (2 children)

I'll try to keep that in mind. What do I do the next time I'm working on a very long branch? The next big project to do on this thing is fixing the myriad localization issues that have been plaguing this project, and that'll mean pruning maybe thousands of lines. Merging my fixes to the main branch was apparently a nightmare (I've already apologized. Profusely. And haven't done it again.)

Should I just try to get as much work done in as few commits as possible?

  • permalink
  • fedilink
  • source
  • parent
  • hideshow 2 child comments
    •
  • [–] 4 points 2 weeks ago (1 child)

    First off, don't sweat it, the "nightmare" was probably 20-30 annoying minutes for the senior.

    But commit count isn't what would've made it tough to merge, and strictly speaking smaller commits is typically better.

    Concretely: limit the kinds and the reach of changes you're asking someone to review & merge. I've never done localization but I'll try to guess for an example -

    1000 lines changed because you're replacing an incorrect punctuation character misused everywhere - that's really only one change to have to think about, it's 1000 lines but one fairly trivial change.

    Now, if each of those lines was improving some phrasing, done by a native speaker that found existing phrasing clunky for their language? Well, each of those are their own judgment call and have to be assessed individually. A thousand fairly subtle changes, and a PR isn't meant to accept some and reject others.

    More abstractly, try to put yourself in the reviewer's shoes (hard when new) - how much stuff do they have to keep in mind in order to make judgment calls about this whole batch of code?

    More if you want it:

    spoilerMoving on from localization, ask yourself, how many "places" in the codebase does your code touch? Are you lumping every single possible thing into a single change set because it feels like a single feature, when it could have been architecturally segmented? Like sure a single PR for a single overall feature makes sense conceptually, but can it be split? Your skill with splitting up tasks is like 70% of your skill as an engineer over time, btw.

    So for example, single feature - all code and PR for DB models, separate PR for API updates, separate PR for UI updates. One feature but the UI folks don't have to think about the other parts, etc. Maybe the ideal case is obvious.

    There are always complicating factors. Some tasks aren't well suited to that segmentation and really require big far reaching changes. Some codebases (bad ones) make it impossible to change one part without touching ~all of it.

    When you're starting out, doing this reliably well requires seniors scoping out tasks properly so that they are developed (and then reviewed) in a sensible, segmented way.

  • permalink
  • fedilink
  • source
  • parent
  • hideshow 1 child comment
    •