4 ms·

Pull requests which have artificial requirements. Which, these days, may mean many things. It's illusion that PR can usually be compact in terms of lines and f

by avmich 4y ago

Pull requests which have artificial requirements. Which, these days, may mean many things. It's illusion that PR can usually be compact in terms of lines and files. That not only requires a good quality of code, which isn't ubiquitous, but also good understanding of code, which changes all the time with different opinions resulting in that code being modified. So PRs become rather large - and sometimes surprisingly missing useful things, because exact change isn't easy. The PR which is hard to review may mean that the code isn't understood the same way by different people, and this is a signal that some discussion is needed. Unfortunately often that's not done, and after some efforts PR is pushed through, while misunderstandings grow.

yellow_lead 4y ago
"Reduce the scope of your PR" a reviewer comments on a PR for a project that hasnt meaningfully addressed its technical debt in years.
- arthurcolle 4y ago
  My last team was like this. So painful
- sshine 4y ago
  I think this may be a valid point, and cleaning up could be seen as a separate concern. The frustrating part is ALWAYS needing to clean up before making any contribution. So following the advice, you always need two PRs.
  - orwin 4y ago
    Depends on the cleanup no? I mean, if it's just obselete version of one library, and the ensuing lockfile change, it doesn't really matter. If iys upgrading to a new language/framework however, yes, please do so.
    - sshine 4y ago
      I'm not saying cleaning up separately isn't warranted. But it isn't always legacy that needs cleaning up. It can also be mess created very recently by active team members. So on the one hand, a separate PR for the cleaning up is warranted, but since it isn't being matched with a separate PR for introducing the mess, the double standard is magnified by introducing more book-keeping for those who clean up after others. For that reason, when working with messy people who are not held accountable, I usually clean up in separate commits in a PR. This way, when looking at the blame for the logical changes that I introduced, and when reviewing, this is separate from the cleanup.

by avmich 4y ago

yellow_lead 4y ago

"Reduce the scope of your PR" a reviewer comments on a PR for a project that hasnt meaningfully addressed its technical debt in years.

arthurcolle 4y ago
My last team was like this. So painful
sshine 4y ago
I think this may be a valid point, and cleaning up could be seen as a separate concern. The frustrating part is ALWAYS needing to clean up before making any contribution. So following the advice, you always need two PRs.
- orwin 4y ago
  Depends on the cleanup no? I mean, if it's just obselete version of one library, and the ensuing lockfile change, it doesn't really matter. If iys upgrading to a new language/framework however, yes, please do so.
  - sshine 4y ago
    I'm not saying cleaning up separately isn't warranted. But it isn't always legacy that needs cleaning up. It can also be mess created very recently by active team members. So on the one hand, a separate PR for the cleaning up is warranted, but since it isn't being matched with a separate PR for introducing the mess, the double standard is magnified by introducing more book-keeping for those who clean up after others. For that reason, when working with messy people who are not held accountable, I usually clean up in separate commits in a PR. This way, when looking at the blame for the logical changes that I introduced, and when reviewing, this is separate from the cleanup.