You can’t write new code if there’s already finished code you could be reviewing instead.
That's a nice way to put it, I haven't heard it before.
the second instance of the same ‘because’ becomes a deep link into project’s style document
That's true, although if it's linter/compiler enforceable, then it might already be enforced. This point is largely about reasoning beyond the code style.
Our style guide tells you not to use abbreviations in names of things. There are a few reasons for this:
If you don't do it 100% of the time with the same abbreviation, people need to track whether you used the abbreviation in this specific case, which leads to all of the problems of inconsistency in APIs.
People have different backgrounds and so an abbreviation that makes sense to you may make someone else assume something incorrect about the code (for example, some of us are pretty confident that IPC means inter-process communication, whereas the hardware folks are convinced it means instructions per clock).
People have different native languages and a natural abbreviation to you may be weird to someone else and confuse them.
Modern code editors (including vim) have autocomplete, you're not saving much typing.
Code is read more than it is written, saving typing is a false economy even if it is real.
This isn't something that you can mechanically check, unfortunately (maybe an LLM could in theory, but typically the tokenisation step breaks it's ability to do so).
LLMs don't see streams of letters, they see streams of tokens. Code LLMs split identifiers into tokens that are not the same as the language-level notion of a token. So it may see a sequence of one-letter tokens for an abbreviation, but it may see a single token within an identifier.
You won't get to where you think you can get by chasing this and that trick. The latest trendy practice. This rule and that procedure to mitigate this and that problem of pitfall. It's a never ending chase.
The fundamental problem arises from the flawed premise that you can ensure quality software by gatekeeping every single voting line. But this doesn't make sense. A good architecture requires understanding of the whole system and a clear idea of what goes where and why things are built the way they are. You don't achieve this by patchwork. Sure, you can have the very best patches, bit it's still patchwork.
People buy in to the mindset of quality by strictness on pull requests, then the whole thing becomes endless and painless silliness about how many spaces, upper or lower case conventions, variable names, linters, code formaters, and what have you. But I'm reality "all these are of secondary importance at best*.
Then there is the fundamental secret sauce. Do you have skilled high competence engineers or not? No amount of review processes will save you if the skill and competence are low. It's not realistic to ignore these. No single employer would give hand of skill and competence in favor of these things.
If you have a super productive engineers that doesn't use your code formating conventions, are these so important after all? Would you interfere with their job for the sake of an extra space after a parens?
When people say that code reviews are a waste of time, I instantly suspect that they've only experienced code reviews in organizations that don't know how to do them. And then I read a line like this that all but confirms it:
People buy in to the mindset of quality by strictness on pull requests, then the whole thing becomes endless and painless silliness about how many spaces, upper or lower case conventions, variable names, linters, code formaters, and what have you.
These aren't things that people should be arguing about in a code review, and if that's what code reviews are to you, you were in an organization that did code reviews badly.
There should be consistent conventions because code is hard to read when it's a mishmash of each person's pet style, but automated tools should be enforcing that, not humans in a code review. Even for things linters can't catch, you should have a style guide that provides an authoritative answer so that it's not something you're arguing about on a per-PR basis.
Code reviews are a lot like an editor's review for authors and journalists. Other people have to maintain the code, so feedback from actual humans who are trying to understand your code can catch issues early. You don't solve this by just having smart engineers. Brilliant writers still have editors because it's valuable to get feedback from readers.
Code reviews are also a great way to percolate good ideas throughout a team. I've learned so much about good development techniques from code reviews, both as the author and the reviewer.
Yeah, the whole discussion honestly feels kind of bizarre to me.
Code review isn't about catching stray spaces, it's about sanity checking your work (can your coworkers understand how this works? does it interact correctly with the rest of the code base? are there better ways to do this? should we implement this at all?).
Is there at least one other person on the project who can understand the code? If a reviewer can read and understand my code, I have some level of confidence that someone else will be able to as well. And that person may be me in two years when I come back and say 'what does this actually do and why?'.
When I write code, I (normally) know what it does and why. I have a whole pile of knowledge unrelated to the text in the file about the context as well as the intended and actual behaviour. One of the biggest benefits of code review is making sure that I have actually communicated that in the code so that the next person to touch the code has it too.
This can be really helpful when the person doing the review is more junior. If they didn't understand what I was doing, it's for at least one of two reasons:
My code is confusing.
There's something that they should learn.
Often both, but the second one gives them an opportunity to see a thing. If I say 'this is the flibbleworble pattern' and they say 'what is a flibbleworble', then there's something that they can learn. If it turns out the flibbleworble pattern was something I just made up, then I probably need a big comment explaining it at the start of the file and they get to learn about it for next time.
And it's worth noting that seniority / experience isn't a total ordering here. Reviewing code from nominally less experienced people is a great way of learning about new ideas that weren't taught when you were learning to program.
The best reviews I've received have been just one or two comments on a thousand line diff, each diving into some arcane but extremely relevant detail of the problem that I didn't previously consider, ending with a set of actionable recommendations.
painless silliness about how many spaces, upper or lower case conventions, variable names, linters, code formaters, and what have you.
If you put the linter in the build pipeline, the PR never even makes it to review because these things will break the build. It's the way we've done it for years at this point. People not automating this stuff in 2025 need a nice polite bonk on the head.
Yeah. Waiting for it to hit CI is silly and takes too much time.
I like them much better as pre-commit checks. For most of my projects, there's a pre-commit hook that runs a linter and fails to commit if it finds anything. It proposes a fix that you can accept by running git add on the files it fixed for you.
I find that not silly at all. It removes all the cognitive load, it's fast, and it's one of the things that makes it easier to orient newcomers to the codebase as time goes by. I guess if you are working in a language that doesn't have an automatic formatter that can do this for you, it's probably not worth it. But doesn't pretty much everything have one of those these days?
It's also a thing that makes future reviews more efficient. Sure, an extra space after a paren doesn't matter much. But, for example, in python, for lists where the items span multiple lines, having a comma after the last one makes future diffs cleaner and reviewing them easier. And not letting someone commit an unused import cuts down on noise. Catching assigned but unused local variables catches bugs. It's all secondary in the sense that you can write (apparently) working code without it, but it's valuable for a codebase that is going to stick around. And a good linter can fix much of it with nothing more than a second git add . && !! in many cases.
Yeah. Waiting for it to hit CI is silly and takes too much time.
The point of having it in CI is to actually enforce whatever standard you are trying to enforce regardless of whether they have pre-commit checks or not. The pre-commit checks are necessary too, unless you want to run them manually or something.
A lot of modern linting tools are really general static analysis tools (rubocop, cargo, golang-ci) that do more than just syntax lints as well. So there's extra value beyond "oops you used tabs instead of spaces" or whatever.
I think we're in violent agreement. I should have emphasized wait more... I was saying it shouldn't wait until CI in reaction to the commenter complaining about how it could be a waste of time. Running it on commit corrects for that quite nicely, IMO.
D'oh I meant clippy instead of cargo and golangci-lint instead of golang-ci. It's been a long week.
By the way, while I'm here might as well add standardrb. We migrated to it, which is rubocop with pre-defined (and unconfigurable, lol) sane defaults for new ruby projects. I hate code-style arguments and glad to see stuff like this.
We migrated to it, which is rubocop with pre-defined (and unconfigurable, lol) sane defaults for new ruby projects. I hate code-style arguments and glad to see stuff like this.
That's part of what I like about black (and ruff) for python. They're almost unconfigurable and so automatic that it's not worth anyone's time to argue. On things like this, people may have their preferences, but I haven't met many who'll maintain a fork of the standard linter to enforce them, and, TBH, having a style automatically enforced so I don't have to discuss it at review time is much more important than what the specific style is, within the universe of styles people actually want, anyway.
On legacy projects I’ve seen pushback on doing One Big Reformat to enable format checking in the CI because it makes it difficult to effectively use git blame or git bisect (if the offending code change was before the reformat). Perhaps this is just a skill issue though.
Unfortunately, I don’t see any built-in similar flag or config for git bisect. But I’m not sure why a formatting commit would significantly affect git bisect anyway. If you are testing whether tests pass, or whether a certain keyword is present in the codebase, a formatting commit would not cause a false positive or negative.
If your team really found the formatting commit too often with git bisect, they could put into the repo a script wrapper for git bisect run. The script would exit with status 125 whenever the current commit is one listed in .git-blame-ignore-revs, and otherwise delegate its status code to the command given as an argument. The cost of this solution would be remembering to use the script wrapper with any command given to git bisect run.
On legacy projects I’ve seen pushback on doing One Big Reformat to enable format
Yeah I wouldn't recommend doing that. Many tools (e.g. rubocop) have where you can save the warnings into an ignore file. Basically a todo list for fixing later that are ignored by the checks. As you fix problems, you remove the entry from the list.
about how many spaces, upper or lower case conventions
One thing to consider is that for most teams “later” doesn’t exist. And tiny pains and inconsistencies can add up over time. And most people don’t tolerate “refactor for consistency” PRs. So the result is: a “now or never” mindset and PRs become one of the only places to get these things.
I have taken to: heavily using linters and deferring style to them. Additional requests by me (if it’s style or convention) will only happen alongside of an inline suggestion. Or if I can’t do that, a stacked PR that implements my change. This changes the convo from “do work for me code monkey” to “I put my time where my mouth is.” These suggestions also come with a “because.” Alternatively I’ll open issues for things that should be done but aren’t urgent and shouldn’t block the PR.
The goal of the team should be to converge on some shared understandings of how to work over time. If you feel someone is missing the plot, talk to them. State how you’re wanting to work instead. And be curious for why they are behaving the way they are. Find out what is important to them, try to find alternative ways to give them what they want that meet your needs too.
Finally: If possible, discuss how a review will happen before it happens. A lot of time that I have crap reviews, I didn’t expect them and have to rapidly ramp up. And then all I can do is nitpick if I don’t understand all of the context. To add: many PRs, especially internal ones skip a TON of context. As in: title only and no description. One word commits etc. if you’re not getting substance in your reviews, help coach them “here’s some things I’m looking for in this review” and ideally include some validation suggestions. Also: review your own code as if it were someone else’s. If all things go well, when a coworker leaves a comment it should be a “thanks for helping me make this better, I should have caught that myself” moment (aspirationally).
According to this talk, code review is about the most proven practices (even if that's because everything else does not seem to have visible results in serious studies).
You are right that wasting time in trivial stuff in code reviews is a waste of time, but my experience has been that you can keep that under control. There's going to be some occasional bikeshedding, but that can be tackled, and those skilled high competence programmers can provide valuable feedback to other skilled high competence programmers. (If they cannot, perhaps they are not so skilled?)
In my experience there is occasionally going to be something that is not bike shedding every now and then, with 95% of the time wasted in whitespace and variable case discussions. This is because it's basically all that mindset and processes lendo themselves to.
For example, In the talk you sent, he starts by a ridiculous premise. It is known that people read the first few paragraphs or pages of anything than cannot be consumed in a few seconds. However that should not apply to work! The premise that people will have that disregard for the core of their work, is unacceptable. If you put reviews as a central part of your workflow, of course it is unacceptable that the reviewers need to read them through. Otherwise what's the point anyways?
A good architecture requires understanding of the whole system and a clear idea of what goes where and why things are built the way they are.
If the codebase is small enough that all parts of it easily fit into the head of a single person, and everyone on the team has been on the same codebase from the beginning, and everyone is aware of every change that goes in, then I agree – you might not need code reviews. If either of these isn't the case, there are quite a few reasons why code reviews can be helpful even if everyone on the team is highly skilled and competent.
I never claimed you dont need code reviews. But code review does not equate to this process that starts and ends in pull requests and a code review for each. That's an extremely limited view on software engineering. If you developed some code as a library, you can review the whole code as is and include it as a dependence if you think it's valuable to your project. That's just an example.
What I am pointing out is that this idea that GitHubs web interface for pull requests (naming that they created) is a software engineering process in itself is crazy.
But to answer your post directly. One does not need to know the whole codebase. Just its architecture. And yes, this is absolutely possible. Specifics can (should) be easy to figure out in useful time for some one that has understanding of the architecture. Even for very large projects.
I have more thoughts on the general topic than this text box can fit, but I just came across this paragraph from an unrelated blog post re the smallest thing of coding conventions:
We’re running close to 5,000 tests at this point. Large scale code refactoring tools aren’t widespread in Go, so I did most of the refactoring with some very gnarly multi-line regexes, and even with those, the only reason that it was possible was that we’re obsessive with keeping strong code convention. Most test cases were structured with an identical layout, which might’ve seemed like unnecessary pedantry when it was first going in, but later paid off in reams as I refactored thousands of tests in hours instead of weeks.
Another simple rule that fixes code reviews: when you post a pull request, tell people which files they should look at first. If you don't do this, people review files in alphabetical order, paying the most attention to the first file they see instead of the files you actually want.
I really like structuring PRs in commit order if possible. It requires time to rewrite history and to learn how to split things out into small chunks but it saves the reviewer time and often while I’m reworking to tell a better commit story, I find improvements to make in the code. This is much easier in rust that other langs like Ruby. It might seem like 20 commits would be annoying, but if most of them can fit on the same page, reviewing them at a glance takes no time. It’s amazing how muddy a commit can get when there’s only two to three “related” changes in there at the same time.
Or instead of files, suggest an entry point like “Start with the test and trace the implementation” if the test is asserting the wrong thing or a meh API that should come before reviewing N files for minor style and implementation decisions.
And for reviewers: Pull down the code and put it in your ide. There’s only so much you can do looking at static strings on a webpage. Syntax highlighting, ability to run tests, make changes to see if they break things etc. is much better when it’s in a development environment.
This is much easier in rust that other langs like Ruby.
I was surprised to see you say this because this is exactly what I do in Ruby and find it quite easy to do. Organizing a PR by commits often means too that I can explain each individual set of changes within the commit messages a lot better. I'm a big fan of this process both when I'm writing and reading a PR.
Refactoring in rust is much easier. Static types make it much safer to assert you didn’t miss anything. You can do the same with Ruby and enough unit tests, but it requires a certain diligence of all prior programmers.
A reminder to also be kind to your fellow reviewers: if it's not absolutely self-explanatory, please make an effort to include the motivations behind a change, whether it be in the PR description, the commit message, or a code comment. I guarantee you that your PR will get reviewed much more quickly if you try to put yourself in the reviewer's shoes by pre-empting questions.
Going deeper on rule two, something I've used in the past as a more formal method is SBAR. It can help to ground the reasoning in objective facts a bit better than a simple "because" does. Otherwise things like "because we do it this way" or "because 'best practice'" tend to creep in.
The version of this advice I’ve heard is “explain the consequences”, which could include “doing it this way makes X example mistake less likely in the future”.
That's true, TIIL about SBAR, thanks. Also, once you write it down, it's at least much easier to catch yourself doing this and stop early. I also posted on what I think about the "best practice" just two months ago, https://lobste.rs/s/wfpw3u/justification_filler_phrases – definitely a phrase to avoid.
I think the author personally suffered mainly in the hands of bad reviewers, because this article is one-sided. Reviewing fast is great, but a bad submitter will easily turn you into a coding tool by getting used to sloppy submissions that you walk them through over a dozen iterations.
So I'd say the most important rule on the submitter side is that you should always make your best attempt at getting your change through without any review comments. Of course we are all human, we make mistakes, we have different preferences and perspectives etc. and it is not a failure to have one or two iterations . But pressing the "request review" button should mean "I made an honest attempt at getting you to approve on your first review.
All it takes is to review the code yourself first and fix any sticking points, architectural smells etc. and then make sure that any correctness concern the reviewer might have is covered by either tight types or tests. And if there's anything left that you can't cover with the CI pipeline, document how you manually confirmed that your code is working, so that the reviewer doesn't have to repeat the same procedure that you already followed before you convinced yourself that the code works (you made sure it works, right?)
It's very disruptive to have to review a giant change set ten times after each half-fix to your review and catch another wave of mistakes that could easily be caught by the author simply by reviewing the code themselves or just trying to run the code.
I think the author personally suffered mainly in the hands of bad reviewers, because this article is one-sided.
As the author, I can say that's not the case. The main reason it's one-sided is that I've acquired a belief the reviewer's side is the only one that really matters in terms of optimising the shipping speed of an organisation. Yes, there are plenty of ways to improve the process from the author's side as well, but the impact is not comparable. In the case of a good submission but a slow/unclear reviewer, there is no recourse. In the case of a sloppy submission and a reviewer who follows these two rules, a few quick rounds of feedback with explicit reasoning can nudge the author in the right direction.
To be clear, are we talking about latency or throughput here? Because in my experience, in a product setting, latency only matters in uncommon cases like "production on fire" bugs and "very important customer has a very urgent need", where I agree, some sort of management person should make it clear that latency should be minimized at all costs.
But the regular operation mode of a product shop should be to maximize throughput, which means respecting the time of the reviewer as much as the reviewee. I'd even argue that in practice, the time of the reviewer tends to be somewhat more important that the reviewee, because review requests tend to go from less experienced devs to more experienced. And reviews tend to take longer when a more experienced reviewer is reviewing a less experienced dev.
In the case of a sloppy submission and a reviewer who follows these two rules, a few quick rounds of feedback with explicit reasoning can nudge the author in the right direction.
You're making a lot of assumptions here.
First, you're assuming that a sloppy submission can be fixed quickly. What if the submission has an unacceptable architectural weakness? Or what if it makes a lot of long-distance assumptions that need to be tested by extending the test setup in non-trivial ways? Even if the reviewer responds immediately, the next iteration might happen the next day, and the reviewer will have to be pulled in again.
Second, you're assuming that these people are working in the same time period, which probably also means in the same time zone. In the case of a distributed team, the soonest time the reviewer could possibly respond might be the next day.
You're also assuming that "a quick round of feedback" is a short amount of time lost for the reviewer. Depending on the task I'm working on, my brain could be hosting a few hours of complex and fragile context. Every time I get interrupted with a review request, I have to reconstruct that context. And if I get interrupted too many times while working on the same complex task, it gets harder and harder to concentrate on it (a kind of learned helpnessless?).
In the case of a good submission but a slow/unclear reviewer, there is no recourse
If you submit a good PR for review, it tends to get approved on the first review. Which means it doesn't matter how long it takes to get reviewed, because you won't have to switch your context back to that task. You can move on to some task on top of the main branch, or if the next task depends on the changes in the submitted PR, you can branch off of that and rebase to main as soon as it gets merged.
I see your point, but I still disagree on many aspects.
To be clear, are we talking about latency or throughput here?
I'd argue that in many ways these two are not independent, and that minimising latency maximises throughput. The costs of context switching and building momentum on a particular work stream can't be underestimated.
the time of the reviewer tends to be somewhat more important that the reviewee, because review requests tend to go from less experienced devs to more experienced
This point unfortunately tends to ignore the culture-building aspect that I tried to encapsulate in this paragraph from the article: "But what's often missed is that the behaviour of senior or staff engineers is what establishes the culture. Other, more junior engineers will see what you're doing and will start repeating your behaviour, but on a small scale. Yes, clearly that big, complex change you're reviewing requires more time. But if it takes you two days to find time to review it, others will treat it as the norm, and now even a small change might take a couple of days too."
What if the submission has an unacceptable architectural weakness?
It gets rejected quickly with good feedback and clear communication what the weakness is. Note I'm not saying that better submissions minimise latency as well, I'm saying that the impact of these two rules heavily outweighs the impact of any other ways to improve the code review process.
If you submit a good PR for review, it tends to get approved on the first review.
Yet when every reviewer prioritises their own flow, e.g. as a consequence of "if I get interrupted too many times while working on the same complex task", the first review can still take a consider amount of time, no matter how good it is.
I like this formulation (learned it from a Jane Street post a while ago):
You can’t write new code if there’s already finished code you could be reviewing instead.
I also like “the second instance of the same 'because' becomes a deep link into project’s style document”.
That's a nice way to put it, I haven't heard it before.
That's true, although if it's linter/compiler enforceable, then it might already be enforced. This point is largely about reasoning beyond the code style.
It's not code style document, style as in "black metal", all big and small brained ideas why code is written in a particular way go there!
Ah, got it – yeah, this approach sounds reasonable.
To give a concrete example:
Our style guide tells you not to use abbreviations in names of things. There are a few reasons for this:
This isn't something that you can mechanically check, unfortunately (maybe an LLM could in theory, but typically the tokenisation step breaks it's ability to do so).
How do you mean the tokenisation breaks it? It can totally check it but it would be slow AF compared to existing “legacy” linters :D
LLMs don't see streams of letters, they see streams of tokens. Code LLMs split identifiers into tokens that are not the same as the language-level notion of a token. So it may see a sequence of one-letter tokens for an abbreviation, but it may see a single token within an identifier.
You won't get to where you think you can get by chasing this and that trick. The latest trendy practice. This rule and that procedure to mitigate this and that problem of pitfall. It's a never ending chase.
The fundamental problem arises from the flawed premise that you can ensure quality software by gatekeeping every single voting line. But this doesn't make sense. A good architecture requires understanding of the whole system and a clear idea of what goes where and why things are built the way they are. You don't achieve this by patchwork. Sure, you can have the very best patches, bit it's still patchwork.
People buy in to the mindset of quality by strictness on pull requests, then the whole thing becomes endless and painless silliness about how many spaces, upper or lower case conventions, variable names, linters, code formaters, and what have you. But I'm reality "all these are of secondary importance at best*.
Then there is the fundamental secret sauce. Do you have skilled high competence engineers or not? No amount of review processes will save you if the skill and competence are low. It's not realistic to ignore these. No single employer would give hand of skill and competence in favor of these things. If you have a super productive engineers that doesn't use your code formating conventions, are these so important after all? Would you interfere with their job for the sake of an extra space after a parens?
When people say that code reviews are a waste of time, I instantly suspect that they've only experienced code reviews in organizations that don't know how to do them. And then I read a line like this that all but confirms it:
These aren't things that people should be arguing about in a code review, and if that's what code reviews are to you, you were in an organization that did code reviews badly.
There should be consistent conventions because code is hard to read when it's a mishmash of each person's pet style, but automated tools should be enforcing that, not humans in a code review. Even for things linters can't catch, you should have a style guide that provides an authoritative answer so that it's not something you're arguing about on a per-PR basis.
Code reviews are a lot like an editor's review for authors and journalists. Other people have to maintain the code, so feedback from actual humans who are trying to understand your code can catch issues early. You don't solve this by just having smart engineers. Brilliant writers still have editors because it's valuable to get feedback from readers.
Code reviews are also a great way to percolate good ideas throughout a team. I've learned so much about good development techniques from code reviews, both as the author and the reviewer.
Yeah, the whole discussion honestly feels kind of bizarre to me.
Code review isn't about catching stray spaces, it's about sanity checking your work (can your coworkers understand how this works? does it interact correctly with the rest of the code base? are there better ways to do this? should we implement this at all?).
And, most importantly:
Is there at least one other person on the project who can understand the code? If a reviewer can read and understand my code, I have some level of confidence that someone else will be able to as well. And that person may be me in two years when I come back and say 'what does this actually do and why?'.
When I write code, I (normally) know what it does and why. I have a whole pile of knowledge unrelated to the text in the file about the context as well as the intended and actual behaviour. One of the biggest benefits of code review is making sure that I have actually communicated that in the code so that the next person to touch the code has it too.
This can be really helpful when the person doing the review is more junior. If they didn't understand what I was doing, it's for at least one of two reasons:
Often both, but the second one gives them an opportunity to see a thing. If I say 'this is the flibbleworble pattern' and they say 'what is a flibbleworble', then there's something that they can learn. If it turns out the flibbleworble pattern was something I just made up, then I probably need a big comment explaining it at the start of the file and they get to learn about it for next time.
And it's worth noting that seniority / experience isn't a total ordering here. Reviewing code from nominally less experienced people is a great way of learning about new ideas that weren't taught when you were learning to program.
The best reviews I've received have been just one or two comments on a thousand line diff, each diving into some arcane but extremely relevant detail of the problem that I didn't previously consider, ending with a set of actionable recommendations.
If you put the linter in the build pipeline, the PR never even makes it to review because these things will break the build. It's the way we've done it for years at this point. People not automating this stuff in 2025 need a nice polite bonk on the head.
You still have to fix whatever doesn't pass. This is very bad and silly use of your precious time.
Yeah. Waiting for it to hit CI is silly and takes too much time.
I like them much better as pre-commit checks. For most of my projects, there's a pre-commit hook that runs a linter and fails to commit if it finds anything. It proposes a fix that you can accept by running
git addon the files it fixed for you.I find that not silly at all. It removes all the cognitive load, it's fast, and it's one of the things that makes it easier to orient newcomers to the codebase as time goes by. I guess if you are working in a language that doesn't have an automatic formatter that can do this for you, it's probably not worth it. But doesn't pretty much everything have one of those these days?
It's also a thing that makes future reviews more efficient. Sure, an extra space after a paren doesn't matter much. But, for example, in python, for lists where the items span multiple lines, having a comma after the last one makes future diffs cleaner and reviewing them easier. And not letting someone commit an unused import cuts down on noise. Catching assigned but unused local variables catches bugs. It's all secondary in the sense that you can write (apparently) working code without it, but it's valuable for a codebase that is going to stick around. And a good linter can fix much of it with nothing more than a second
git add . && !!in many cases.The point of having it in CI is to actually enforce whatever standard you are trying to enforce regardless of whether they have pre-commit checks or not. The pre-commit checks are necessary too, unless you want to run them manually or something.
A lot of modern linting tools are really general static analysis tools (rubocop, cargo, golang-ci) that do more than just syntax lints as well. So there's extra value beyond "oops you used tabs instead of spaces" or whatever.
I think we're in violent agreement. I should have emphasized wait more... I was saying it shouldn't wait until CI in reaction to the commenter complaining about how it could be a waste of time. Running it on commit corrects for that quite nicely, IMO.
D'oh I meant clippy instead of cargo and golangci-lint instead of golang-ci. It's been a long week.
By the way, while I'm here might as well add standardrb. We migrated to it, which is rubocop with pre-defined (and unconfigurable, lol) sane defaults for new ruby projects. I hate code-style arguments and glad to see stuff like this.
That's part of what I like about black (and ruff) for python. They're almost unconfigurable and so automatic that it's not worth anyone's time to argue. On things like this, people may have their preferences, but I haven't met many who'll maintain a fork of the standard linter to enforce them, and, TBH, having a style automatically enforced so I don't have to discuss it at review time is much more important than what the specific style is, within the universe of styles people actually want, anyway.
On legacy projects I’ve seen pushback on doing One Big Reformat to enable format checking in the CI because it makes it difficult to effectively use git blame or git bisect (if the offending code change was before the reformat). Perhaps this is just a skill issue though.
There is a way to prevent formatting commits appearing in
git blame: How to exclude commits from git blame.Unfortunately, I don’t see any built-in similar flag or config for
git bisect. But I’m not sure why a formatting commit would significantly affectgit bisectanyway. If you are testing whether tests pass, or whether a certain keyword is present in the codebase, a formatting commit would not cause a false positive or negative.If your team really found the formatting commit too often with
git bisect, they could put into the repo a script wrapper forgit bisect run. The script would exit with status 125 whenever the current commit is one listed in.git-blame-ignore-revs, and otherwise delegate its status code to the command given as an argument. The cost of this solution would be remembering to use the script wrapper with any command given togit bisect run.Yeah I wouldn't recommend doing that. Many tools (e.g. rubocop) have where you can save the warnings into an ignore file. Basically a todo list for fixing later that are ignored by the checks. As you fix problems, you remove the entry from the list.
One thing to consider is that for most teams “later” doesn’t exist. And tiny pains and inconsistencies can add up over time. And most people don’t tolerate “refactor for consistency” PRs. So the result is: a “now or never” mindset and PRs become one of the only places to get these things.
I have taken to: heavily using linters and deferring style to them. Additional requests by me (if it’s style or convention) will only happen alongside of an inline suggestion. Or if I can’t do that, a stacked PR that implements my change. This changes the convo from “do work for me code monkey” to “I put my time where my mouth is.” These suggestions also come with a “because.” Alternatively I’ll open issues for things that should be done but aren’t urgent and shouldn’t block the PR.
The goal of the team should be to converge on some shared understandings of how to work over time. If you feel someone is missing the plot, talk to them. State how you’re wanting to work instead. And be curious for why they are behaving the way they are. Find out what is important to them, try to find alternative ways to give them what they want that meet your needs too.
Finally: If possible, discuss how a review will happen before it happens. A lot of time that I have crap reviews, I didn’t expect them and have to rapidly ramp up. And then all I can do is nitpick if I don’t understand all of the context. To add: many PRs, especially internal ones skip a TON of context. As in: title only and no description. One word commits etc. if you’re not getting substance in your reviews, help coach them “here’s some things I’m looking for in this review” and ideally include some validation suggestions. Also: review your own code as if it were someone else’s. If all things go well, when a coworker leaves a comment it should be a “thanks for helping me make this better, I should have caught that myself” moment (aspirationally).
According to this talk, code review is about the most proven practices (even if that's because everything else does not seem to have visible results in serious studies).
You are right that wasting time in trivial stuff in code reviews is a waste of time, but my experience has been that you can keep that under control. There's going to be some occasional bikeshedding, but that can be tackled, and those skilled high competence programmers can provide valuable feedback to other skilled high competence programmers. (If they cannot, perhaps they are not so skilled?)
In my experience there is occasionally going to be something that is not bike shedding every now and then, with 95% of the time wasted in whitespace and variable case discussions. This is because it's basically all that mindset and processes lendo themselves to.
For example, In the talk you sent, he starts by a ridiculous premise. It is known that people read the first few paragraphs or pages of anything than cannot be consumed in a few seconds. However that should not apply to work! The premise that people will have that disregard for the core of their work, is unacceptable. If you put reviews as a central part of your workflow, of course it is unacceptable that the reviewers need to read them through. Otherwise what's the point anyways?
More folks need to be comfortable with finishing an hour-long slog of a review and realising that unqualified approval is all they have to give. :)
If the codebase is small enough that all parts of it easily fit into the head of a single person, and everyone on the team has been on the same codebase from the beginning, and everyone is aware of every change that goes in, then I agree – you might not need code reviews. If either of these isn't the case, there are quite a few reasons why code reviews can be helpful even if everyone on the team is highly skilled and competent.
I never claimed you dont need code reviews. But code review does not equate to this process that starts and ends in pull requests and a code review for each. That's an extremely limited view on software engineering. If you developed some code as a library, you can review the whole code as is and include it as a dependence if you think it's valuable to your project. That's just an example.
What I am pointing out is that this idea that GitHubs web interface for pull requests (naming that they created) is a software engineering process in itself is crazy.
But to answer your post directly. One does not need to know the whole codebase. Just its architecture. And yes, this is absolutely possible. Specifics can (should) be easy to figure out in useful time for some one that has understanding of the architecture. Even for very large projects.
I have more thoughts on the general topic than this text box can fit, but I just came across this paragraph from an unrelated blog post re the smallest thing of coding conventions:
https://brandur.org/fragments/parallel-test-bundle
Another simple rule that fixes code reviews: when you post a pull request, tell people which files they should look at first. If you don't do this, people review files in alphabetical order, paying the most attention to the first file they see instead of the files you actually want.
I really like structuring PRs in commit order if possible. It requires time to rewrite history and to learn how to split things out into small chunks but it saves the reviewer time and often while I’m reworking to tell a better commit story, I find improvements to make in the code. This is much easier in rust that other langs like Ruby. It might seem like 20 commits would be annoying, but if most of them can fit on the same page, reviewing them at a glance takes no time. It’s amazing how muddy a commit can get when there’s only two to three “related” changes in there at the same time.
Or instead of files, suggest an entry point like “Start with the test and trace the implementation” if the test is asserting the wrong thing or a meh API that should come before reviewing N files for minor style and implementation decisions.
And for reviewers: Pull down the code and put it in your ide. There’s only so much you can do looking at static strings on a webpage. Syntax highlighting, ability to run tests, make changes to see if they break things etc. is much better when it’s in a development environment.
I was surprised to see you say this because this is exactly what I do in Ruby and find it quite easy to do. Organizing a PR by commits often means too that I can explain each individual set of changes within the commit messages a lot better. I'm a big fan of this process both when I'm writing and reading a PR.
Refactoring in rust is much easier. Static types make it much safer to assert you didn’t miss anything. You can do the same with Ruby and enough unit tests, but it requires a certain diligence of all prior programmers.
A reminder to also be kind to your fellow reviewers: if it's not absolutely self-explanatory, please make an effort to include the motivations behind a change, whether it be in the PR description, the commit message, or a code comment. I guarantee you that your PR will get reviewed much more quickly if you try to put yourself in the reviewer's shoes by pre-empting questions.
Going deeper on rule two, something I've used in the past as a more formal method is SBAR. It can help to ground the reasoning in objective facts a bit better than a simple "because" does. Otherwise things like "because we do it this way" or "because 'best practice'" tend to creep in.
The version of this advice I’ve heard is “explain the consequences”, which could include “doing it this way makes X example mistake less likely in the future”.
That's true, TIIL about SBAR, thanks. Also, once you write it down, it's at least much easier to catch yourself doing this and stop early. I also posted on what I think about the "best practice" just two months ago, https://lobste.rs/s/wfpw3u/justification_filler_phrases – definitely a phrase to avoid.
I think the author personally suffered mainly in the hands of bad reviewers, because this article is one-sided. Reviewing fast is great, but a bad submitter will easily turn you into a coding tool by getting used to sloppy submissions that you walk them through over a dozen iterations.
So I'd say the most important rule on the submitter side is that you should always make your best attempt at getting your change through without any review comments. Of course we are all human, we make mistakes, we have different preferences and perspectives etc. and it is not a failure to have one or two iterations . But pressing the "request review" button should mean "I made an honest attempt at getting you to approve on your first review.
All it takes is to review the code yourself first and fix any sticking points, architectural smells etc. and then make sure that any correctness concern the reviewer might have is covered by either tight types or tests. And if there's anything left that you can't cover with the CI pipeline, document how you manually confirmed that your code is working, so that the reviewer doesn't have to repeat the same procedure that you already followed before you convinced yourself that the code works (you made sure it works, right?)
It's very disruptive to have to review a giant change set ten times after each half-fix to your review and catch another wave of mistakes that could easily be caught by the author simply by reviewing the code themselves or just trying to run the code.
As the author, I can say that's not the case. The main reason it's one-sided is that I've acquired a belief the reviewer's side is the only one that really matters in terms of optimising the shipping speed of an organisation. Yes, there are plenty of ways to improve the process from the author's side as well, but the impact is not comparable. In the case of a good submission but a slow/unclear reviewer, there is no recourse. In the case of a sloppy submission and a reviewer who follows these two rules, a few quick rounds of feedback with explicit reasoning can nudge the author in the right direction.
OK, there are a few points to unpack here:
To be clear, are we talking about latency or throughput here? Because in my experience, in a product setting, latency only matters in uncommon cases like "production on fire" bugs and "very important customer has a very urgent need", where I agree, some sort of management person should make it clear that latency should be minimized at all costs.
But the regular operation mode of a product shop should be to maximize throughput, which means respecting the time of the reviewer as much as the reviewee. I'd even argue that in practice, the time of the reviewer tends to be somewhat more important that the reviewee, because review requests tend to go from less experienced devs to more experienced. And reviews tend to take longer when a more experienced reviewer is reviewing a less experienced dev.
You're making a lot of assumptions here.
First, you're assuming that a sloppy submission can be fixed quickly. What if the submission has an unacceptable architectural weakness? Or what if it makes a lot of long-distance assumptions that need to be tested by extending the test setup in non-trivial ways? Even if the reviewer responds immediately, the next iteration might happen the next day, and the reviewer will have to be pulled in again.
Second, you're assuming that these people are working in the same time period, which probably also means in the same time zone. In the case of a distributed team, the soonest time the reviewer could possibly respond might be the next day.
You're also assuming that "a quick round of feedback" is a short amount of time lost for the reviewer. Depending on the task I'm working on, my brain could be hosting a few hours of complex and fragile context. Every time I get interrupted with a review request, I have to reconstruct that context. And if I get interrupted too many times while working on the same complex task, it gets harder and harder to concentrate on it (a kind of learned helpnessless?).
If you submit a good PR for review, it tends to get approved on the first review. Which means it doesn't matter how long it takes to get reviewed, because you won't have to switch your context back to that task. You can move on to some task on top of the
mainbranch, or if the next task depends on the changes in the submitted PR, you can branch off of that and rebase tomainas soon as it gets merged.There's no excuse for sloppiness.
I see your point, but I still disagree on many aspects.
I'd argue that in many ways these two are not independent, and that minimising latency maximises throughput. The costs of context switching and building momentum on a particular work stream can't be underestimated.
This point unfortunately tends to ignore the culture-building aspect that I tried to encapsulate in this paragraph from the article: "But what's often missed is that the behaviour of senior or staff engineers is what establishes the culture. Other, more junior engineers will see what you're doing and will start repeating your behaviour, but on a small scale. Yes, clearly that big, complex change you're reviewing requires more time. But if it takes you two days to find time to review it, others will treat it as the norm, and now even a small change might take a couple of days too."
It gets rejected quickly with good feedback and clear communication what the weakness is. Note I'm not saying that better submissions minimise latency as well, I'm saying that the impact of these two rules heavily outweighs the impact of any other ways to improve the code review process.
Yet when every reviewer prioritises their own flow, e.g. as a consequence of "if I get interrupted too many times while working on the same complex task", the first review can still take a consider amount of time, no matter how good it is.
I totally agree on this.
I buy that because building and review are separated by a wait state, and minimizing that overhead has lots of well known benefits.