3 ms·
Style nits aside, I don't think it's a good idea to selectively pick the worst N things in a PR and let the rest go. If you're consistently finding 10+ things t
by Sodman 6y ago
Style nits aside, I don't think it's a good idea to selectively pick the worst N things in a PR and let the rest go. If you're consistently finding 10+ things to request changes on in somebody's pull requests, you probably need to look at your engineering process. Was the high level approach discussed before it was fully coded and "ready for review"? This is something easily fixed by process changes.
Are you finding a lot of little things in the PR itself? Many of the usual suspects can and should be automated away (linting, codegen, test coverage reports, static analysis, etc). Others can easily be added to a checklist before PRs are submitted for review, eg "Have you updated the relevant documentation".
Usually newer employees will get more comments on their PRs, as they're not as familiar with the system and might fall into more traps that others know about from experience. This can be mitigated somewhat by pairing/mentoring.
Sure some people just aren't going to cut it on your team, but most engineers when corrected on an individual coding mistake won't repeatedly make the same mistake, in my experience. If you just never mention the mistake, they'll keep making it in every PR and be completely unaware. Sometimes it helps to communicate out of band (ie, not in "official" comments on the PR itself) if there are some low hanging gotchas you want to make the engineer aware of.