3 ms·

PRs on my team are done at the request of the author, who maintains full responsibility for the code. When review is complete, the author merges when they are r

by hnruss 9y ago

PRs on my team are done at the request of the author, who maintains full responsibility for the code. When review is complete, the author merges when they are ready (they have full access rights). The author has the right to ignore whatever feedback they want, since they will share the burden of whatever bugs are introduced. However, merging bugs knowingly and/or ignoring feedback is frowned upon. Reviewers are expected to provide whatever type of review the author requests, including manual testing and review of system impacts. Their comments do not require a response from the author, unless they ask questions. It is frowned upon to comment too much on code formatting. In practice, our developers submit PRs for just about everything. Most PRs are small, and most feedback is addressed. Reviewers are thanked and given kudos for finding issues. Knowledge is shared, and the codebase is improved. I’d estimate that 90% of our bugs are caught in review. Product quality has improved drastically since we implemented this practice. Developers consistently state that our PR process is their favorite part of the job. They get to share their work, get feedback, and become better developers.

mkempe 9y ago
I've had the same positive experience working like that in a small team. In addition, I shared pre-commit git hooks set up to run a few tools that a) enforced the team's code format and documentation standard; and b) performed static code analysis. This mostly prevented code reviews from devoting time to code format and basic documentation issues.
Cthulhu_ 9y ago
> It is frowned upon to comment too much on code formatting. Do you have standards for those though? Like gofmt, prettier, ide formatting configs, etc? I and many others can be rather ocd about maintaining a consistent code style, although I will admit that it's far too easy to focus on that in code reviews (low-hanging fruit). Which is why it should be automatically enforced by tooling so it isn't a distraction during reviews.
- hnruss 9y ago
  > Do you have standards for those though? Yes, we try to follow industry standards for code formatting as much as possible, but we also try not to be too picky about formatting if it doesn't meet those standards exactly. Usually authors will make the same formatting mistakes multiple times, so rather than comment on each and every badly formatted line, we try to comment on the type of mistake once and note that there are other instances in the same PR. I agree that auto-formatting is the way to go. The big thing holding us back from using those tools is that we'd first need to reformat the existing code, otherwise the we'd wind up with a lot of unrelated formatting changes in our PRs (assuming that whole files are formatted when saved, rather than just new lines). Certainly not an impossible feat, just something we haven't done yet.

by hnruss 9y ago

mkempe 9y ago

I've had the same positive experience working like that in a small team. In addition, I shared pre-commit git hooks set up to run a few tools that a) enforced the team's code format and documentation standard; and b) performed static code analysis. This mostly prevented code reviews from devoting time to code format and basic documentation issues.

Cthulhu_ 9y ago

> It is frowned upon to comment too much on code formatting. Do you have standards for those though? Like gofmt, prettier, ide formatting configs, etc? I and many others can be rather ocd about maintaining a consistent code style, although I will admit that it's far too easy to focus on that in code reviews (low-hanging fruit). Which is why it should be automatically enforced by tooling so it isn't a distraction during reviews.

hnruss 9y ago
> Do you have standards for those though? Yes, we try to follow industry standards for code formatting as much as possible, but we also try not to be too picky about formatting if it doesn't meet those standards exactly. Usually authors will make the same formatting mistakes multiple times, so rather than comment on each and every badly formatted line, we try to comment on the type of mistake once and note that there are other instances in the same PR. I agree that auto-formatting is the way to go. The big thing holding us back from using those tools is that we'd first need to reformat the existing code, otherwise the we'd wind up with a lot of unrelated formatting changes in our PRs (assuming that whole files are formatted when saved, rather than just new lines). Certainly not an impossible feat, just something we haven't done yet.