983
Peer reviewing is torture now (europe.pub)
submitted 2 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
[–] 6 points 2 weeks ago* (last edited 2 weeks ago) (7 children)

Jeez. Now I feel bad. I've been working on a project with some new people, and I've never used Github before. I'm still learning the etiquette.

I made a branch, and spent a month viciously hunting every bug I could find. I don't trust AI, so I was doing it all by hand. Dawn to dusk, I was staring at code and typing like I had a fever and the only cure was figuring out where tf that invalid scope is supposed to go.

This is my first real project with other people, so of course I'm so proud when I send the PR and it has 40,000 lines added and 60,000 lines removed. I worked really hard on it, and it sorely needed the update.

It was almost all bug fixes, the actual new stuff was about 1,000 or so lines. But what should I do in the future? I don't wanna be an asshole, I wanna be helpful.

  • permalink
  • fedilink
  • source
  • parent
  • hideshow 7 child comments
    •
  • [–] 5 points 2 weeks ago* (3 children)

    Generally it's more considerate to submit smaller batches of (self-contained!) work at a time. Sometimes the work or existing code is so interconnected that this can't really be done, though. You mentioned spending a month, that's also a long time in most environments to be off working on bulk changes for later review. A week is probably long enough in most cases, some orgs even prefer to push (if not review) code daily.

    Don't agonize over it though, wanting to improve your impact on others while working is the right direction to point, keep walking that way and you'll do awesome, let it develop over time. As in, don't let your desire for politeness slow down your work or growth too much (I do this lol, why I'm mentioning).

  • permalink
  • fedilink
  • source
  • parent
  • hideshow 3 child 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
    •