41–50 of 80 posts
"This code works when you run it" is the last thing I care about when I'm reading the code - it's a given, we won't allow change to merge if it wasn't the case. Code reviews are there to catch mistakes in design, copypasta, how the change fits into the product overall.
> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! You should run it somehow, but if you've got CI running tests, which I think you should have anyways, then does it matter?
Just because there are tests doesn't mean the tests are any good. I've only started my quest in TDD and I've discovered it is rather trivial to write tests that don't really mean anything. I find myself asking "what is this supposed to do?" and "does this actually do what it is supposed to do?". I tend to drift into a design mindset during test writing (more-so than in code review) that highlights data flow and the b…
Earlier quoted context omitted.
Can you expand on this?
You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it…
> Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! Our CI automatically deploys a "review app" when someone creates a pull request. This simplifies code review, QA and demoing!
It sounds cool that you create on-demand review app. In a small scale it works. In a larger scale development is absolute impossible due to resource constraints to create on-demand environment for review app. For instance, in a warehouse automation infrastructure that has 30 devs, working on 8 feature simulataneously, it is next to impossible to create review app with the resources they need.
Earlier quoted context omitted.
Can you expand on this?
You should pull the PR you're reviewing down into your IDE and you should spend like, an hour reviewing it carefully and trying to break it. Have a mental checklist: say they renamed a function ... is the file named the same as the function? Did they remember to rename the file? Did they delete parts of the code? Did they remember to delete everything related to that? Often the most challenging parts are negative: it…
If you don't trust your tests, write better tests.
If you don't trust your migrations, write better tests.
I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at…
Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?
I had the pleasure of being at a startup with a very talented SRE named M. Harrison. His minimum-level review criteria were that when you were planning to approve a PR, you should summarize everything the PR does in the approval comment. Responses like ":+1:" or "LGTM" were not allowed. When I would read a PR intending to understand and then summarize, the errors or omissions (eventually) started to just jump out at…
Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?
Code reviews are not very good: - Most style issues should be caught by automated tools - Most functional issues should be caught by tests (written by another person preferably) - To bring someone up to speed on your chosen style, pair programming is much faster than code reviews - To have shared knowledge of code (increased bus factor) pair programming, or code walkthroughs are much faster Not sure why we have falle…
Earlier quoted context omitted.
Interesting. If before you weren't intending to understand and be able to summarize before, what were you doing when you reviewed code?
Writing the summary is a higher bar than just deciding that you could.