3 ms·
> For speed and quality, PRs should be maintained under 200 lines—with 50 lines being ideal. Personally, I've never been comfortable signing off changes that d
by michaelt 3y ago
> For speed and quality, PRs should be maintained under 200 lines—with 50 lines being ideal.
Personally, I've never been comfortable signing off changes that don't correctly and fully implement the requirements.
If I see a function has something that might be a security bug, depending on how it's called from elsewhere in the codebase, how am I supposed to recognise it as a bug or otherwise, when the code to call it hasn't even been written yet?
- raziel2p 3y agoWe should probably be doing periodic full codebase reviews anyway, regardless of how PRs are being made. It's only our tools that encourage us to purely look at the diff of a PR rather than check out the branch and think more big picture. You could also encourage/force people to write design documents or such before starting work on the actual PRs. Though, a PR that introduces dead code (function that isn't called anywhere) wouldn't get approved by me :)
- from-nibly 3y agoThat never happens. There are like 100 things that need to happen that never happen that should happen before doing a whole code base review. Reviews either happen before it's merged or they never happen.
- aseipp 3y ago> how am I supposed to recognise it as a bug or otherwise, when the code to call it hasn't even been written yet? You would just leave a comment on the change, and ask for the author to explain themselves? Or propose they fix it? I mean, that's the entire reason you're doing the code review?