Code Review Handbook
sledgeworx.io
Code Review Handbook
1–10 of 51 posts
Re: Code Review Handbook
#2Nice article! I agree with all your points, they closely mirror my experience too!
Re: Code Review Handbook
#3Nice article! I agree with all your points, they closely mirror my experience too!
Same here, there is something reassuring to see that it is quite common!
Re: Code Review Handbook
#4If I had a dollar for each time a merge request was reviewed by one of my peers and all they said "it looks good to me", I would have a small fortune.
Re: Code Review Handbook
#5If I had a dollar for each time a merge request was reviewed by one of my peers and all they said "it looks good to me", I would have a small fortune.
Weren't you paid for creating those reviews? :)
Re: Code Review Handbook
#6The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers.
If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them on file saves if possible). Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR. Force pushes and skipping hooks should be vigorously discouraged. That way badly formatted code shouldn't make it to the repo and your PRs can focus on more important things.
Obviously this is really hard to implement on a project that's old and has thousands of linter errors. If you didn't set it up on a greenfield project right at the start you can't really complain too much though.
Re: Code Review Handbook
#7It's interesting that the author puts "downloading the code" in the exhaustive section.
Is it really that much work to run git checkout? I routinely checkout code in PRs if I'm not familiar with that part of the codebase. I can navigate the codebase much quicker in my editor than in GitHub. Although GitHub is actually getting there with the semantic analysis of code and finding references and stuff, sometimes I just need to open 5 files in different splits to figure out what is going on.
Re: Code Review Handbook
#8I think I disagree with the emphasis on asking questions. Not because questions shouldn't be asked, but because they should be asked much earlier in the process than the PR stage. Every feature should at a minimum have a brief doc for the requirements and high level design. If you haven't read that and are reviewing the PR for anything other than style, you need to take a step back and go read that doc first.
In my opinion, the levels are:
1. Barely-existent review: The PR author is the SME, I'm only reviewing and LGTMing this because peer review is needed to submit the change. I might notice a typo or something along those lines, and will call out if they're being lazy and skipping tests.
2. Functionality-focused change: Does the code meet the objective and do the tests adequately cover the change?
3. In-depth review: Does the code address the problem in an efficient way that is not likely to cause the team future pain?
Re: Code Review Handbook
#9The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them o…
> Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR
I'd rather people check in their WIP than have it sitting on their machine. Let GH actions worry about whether the code is formatted correctly.
Re: Code Review Handbook
#10The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them o…
I think that often it's not just coding style (yes, a linter should catch indentation errors), but coding conventions/requirements to enforce an idiomatic code structure in the code base. Linters can't catch that. E.g. how to organize files, where to put business logic, how tests are structured, which libraries to use, how to handle non-blocking or concurrent code, errors, code documentation etc.