10 ms·

PR process killing morale and productivity

145 points by devslovecoffee 2y ago

WesolyKubeczek 2y ago
Per the article, not the pull requests itself, but the tedious bikeshedding around them. The article's headline is a bit misleading.
- rob74 2y ago
  That's probably what they mean by "PR Process"?
  - WesolyKubeczek 2y ago
    "Your" got editorialized away and the title became quite categorical. Implying all of them kill morale and are flawed.
DragonStrength 2y ago
Watched a guy get fired mostly because he made code review such a nightmare for everyone else while getting lapped by everyone in terms of actual output. I’m not sure there’s much to be done once you’ve hit “toxic.” The only question is whether the standard can be reset for the whole team.
treespace8 2y ago
GIT seams to be optimized for network of trust. With one person at the top approving what gets merged into the release. This person of course does not do all of the verification, other then broad strokes of what the change does, and who wrote it, reviewed it and tested it. I feel like companies do not want a large tree like structure for their development teams. Without a network of trust it can become mob rule, which is what this article appears to be describing.
- yjftsjthsd-h 2y ago
  That makes sense; https://en.wikipedia.org/wiki/Conway%27s_law https://en.wikipedia.org/wiki/Conway%27s_law applies and git was created by Linus Torvalds for Linux, which works exactly like that.
barbazoo 2y ago
> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.
- znpy 2y ago
  on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you to do your best work. You will be essentially blocked pending their approval (so that nitpicks are changed according to their likings). The weird thing is that all this traceability leaves traces for management to see who's doing a good pr review job and who's not... But I've learned that management usually does not care much.
  - prh8 2y ago
    "will approve after nitpicks" is the most asinine behavior
  - barbazoo 2y ago
    Agree. Why did the junior developer feel like their changes were ready to be reviewed. Looks like they had little guidance. Why didn't a senior developer suggest to close the PR, apply the formatting or whatever and then re-open the PR or something like that. 300 comments, that's either a lot of developers commenting or just a lot of back and forth. I don't get how anyone would let it come to that point. Makes me feel grateful for the places I worked at and the experience I have now.
  - dec0dedab0de 2y ago
    * I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter).* I always thought this was the best way. I wish these systems had a way to assign severity to comments, and urgency to the commit. If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.
prh8 2y ago
I've worked with plenty of devs who embody the toxicity here, lots of stylistic nitpicks and don't see the big picture. All the noise generated distracts from catching the important stuff.
Joker_vD 2y ago
...just how big was that "first ever contribution" pull request that it gathered 300 comments? My first commit in the current company was a 2-liner addition to an existing function plus another 20 lines for a test that verified that indeed, the change does affect the outcome.
- barbazoo 2y ago
  Even a simple (and very appropriate for a junior/newbie) change like you suggested could invite dozens of comments if the engineers don't know what they're doing.
  - Joker_vD 2y ago
    "Dozens" means "at least 24", so that'd been slightly more than 2 comments per line in my example. Which is an insane amount of comments by any standard imaginable: because long before you reach 1-1 comments-to-code ratio, you'd stop, and write a single "no, all of this is a wholly incorrect approach, complete re-do is needed" comment instead.
    - mckn1ght 2y ago
      You’re right, but some people out there will flex and grandstand in their reviews if they operate adversarially instead of collaboratively, which is what your suggestion would encourage.
zie1ony 2y ago
Recently I allowed everyone in my team to push to develop without PRs and even without doing feature branches. We review the code all together (max 2 ppl) just before the release and that's it. But great CI pipeline is must for this. It will get even better soon when AI will be able to slightly refactor the code.
- lapcat 2y ago
  > We review the code all together (max 2 ppl) just before the release and that's it. Is there a reason why you don't review the code immediately? You could also do that without PRs.
  - pavel_lishin 2y ago
    Yeah, I've heard arguments for this, and it always feels weird to me. Why go so long without review? That's just giving developers - junior and senior - opportunity to dig themselves into a hole that they don't notice until someone points out, "hey, your last five commits are buggy, but that matters less than the fact that they're implementing the wrong thing."
    - zie1ony 2y ago
      Good point, but my team is small and has only top engineers with both soft and hard skills. Many times you can do much more with team of 3 than team of 10 ppl.
      - Aeolun 2y ago
        If the three sit next to one another I agree that PR’s are overkill. If they’re remote it becomes harder I think.
BurningFrog 2y ago
Another angle is that a PR with over 300 comments is probably way too big. Many small focused PRs is IMAO much better than big PRs with weeks or months of work in them. But that requires fast and friendly PR reviews!
- eddd-ddde 2y ago
  It also requires good tooling and some experience. GitHub absolutely sucks at making multi-commit features. At least last time I used it. And getting people to actually make smaller commits and reviews is incredibly hard.
  - herpdyderp 2y ago
    Can confirm GitHub's process is poor for small PRs (if that's what you mean by "multi-commit features"). My team has mostly gotten around this with custom CLI tools (for pushing small PRs chained together) and web apps (for concisely viewing your code review status, both giving and receiving).
    - kleinsch 2y ago
      I’m a huge fan of https://github.com/VirtusLab/git-machete https://github.com/VirtusLab/git-machete
      - mckn1ght 2y ago
        Looks great, like a combination of the gh cli and the many options available for maintaining chains of stacked branches!
gboss 2y ago
Whenever I encounter a pull request that I find many issues with, I ask to meet with the engineer and review it one on one. More than half of the workplace problems engineers have is due to their introverted nature and their refusal to get on a teams/zoom call to explain their issues and get resolution. Comments are a really poor mechanism for teaching programming best practices.
- BurningFrog 2y ago
  At my last job, the rule was if the PR is big enough, get on a call and go over it in person. ideally with more than one "reviewer", to break stalemates. Worked really well! I used to be that introverted guy, but a few years of pair programming completely cured that, and I'm now very comfortable discussing design in detail and at length, in a (if I may say so myself) friendly and constructive way. If you've never discussed design with other people, it's a genuinely difficult thing to do!
  - skydhash 2y ago
    A PR big enough should probably have its changes implemented and reviewed unit by unit on a feature branch before its creation. Or it should be an already approved work (refactoring, reformatting,...)
    - t-writescode 2y ago
      And if it's refactoring / reformatting, then it should be pair-programmed to validate that exactly what was said to have been done was done and the pair programmer should confirm such in the PR.
  - mckn1ght 2y ago
    I would love this approach, but the people reviewing my PRs are 10 tzs ahead of me, so we basically can’t meet synchronously unless someone is willing to meet early in the morning or late in the evening. Maybe prerecorded code walkthroughs would help here, but nothing replaces instant face to face pairing. That’s where you can provide somewhat nitpicky feedback but with the empathy that comes with our instinctual responses to body language and voice tone… which over time builds a much better culture IMO.
barbazoo 2y ago
I recommend using something like https://github.com/erikthedeveloper/code-review-emoji-guide https://github.com/erikthedeveloper/code-review-emoji-guide in code reviews to signify what your expectation is exactly.
- SoKamil 2y ago
  Can you share your experience with this kind of review process?
  - barbazoo 2y ago
    I can mirror what their docs say > Using CREG (Code Review Emoji Guide) puts more ownership on the reviewer to give the reviewee added context and clarity to follow up on code review. For example, knowing whether something really requires action (), highlighting nit-picky comments (), flagging out of scope items for follow-up () and clarifying items that don’t necessarily require action but are worth saying ( , , )
jchw 2y ago
Personally I have a strong distaste for projects that try to use some metric for how long a function should be, e.g. line count or cyclomatic complexity. I'm not sure if this one is better automated, human judgement for what makes sense seems better to me. Sometimes the cleanest and highest performance way to write some code is going to be basically one relatively large function. If there's a function complexity/size lint I'll often chunk off helper functions that are virtually useless outside of that specific function, and in doing so, often make it harder to follow what's going on. This isn't universally true of course, but it's a "when not if" situation if you have a hard lint limit and it's not very high. (e.g. In my opinion if you're going to have a lint for this, set it to something rather obscene, something that would be extremely hard to cross without doing something clearly awful. In my opinion it's always been set at least 4 times lower than it should be.)
- Kinrany 2y ago
  It shouldn't be a hard rule, but in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context. All a linter does automatically is require adding the "ignore this line" comments that are then visible to humans in diffs.
  - bryanlarsen 2y ago
    A linter directive also tells humans that somebody has decided that the function being long is acceptable in this specific location. It's been acknowledged. In many cases, that's all I want.
    - mckn1ght 2y ago
      Then you go from pre-linter arguments about line/function/class length to arguments over whether it’s appropriate to override the linter rule in those same instances.
  - bsder 2y ago
    > in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context. And will make understanding the function a lot more difficult. If you want an example of this, look at any C++ code that uses DX12 or Vulkan. It will be atomized sufficiently that you'll have to hunt through dozens of files to figure out which constructor/destructor where something fired. Part of the problem is that C++ discourages naked functions operating on structs rather than member functions on classes. Whereas, if those functions were still all linear in a single function, you'd find what you were looking for in the same file you were already in. That having been said, I'll probably organize that big function as though it were separate functions. But, then, I'm not allergic to adding an extra block level here or there which GASP might exceed 80 columns.
donatj 2y ago
My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've had unrealistic dreams like holding people responsible for things they approved that had bugs, but anything like that would be supremely unpopular. I could do something like requiring review from the team members that actually review, but they already feel overwhelmed by being the only ones that actually review. We tried setting soft limits on the size of PRs, but that comes with a lot of PR that are hard to review because the work is poorly divided and doesn't make sense in isolation.
- simmschi 2y ago
  Have you tried the code owners feature (assuming you're on Github). IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-) If you don't have specific ownership in your code base I'd start there.
  - donatj 2y ago
    This is all within a relatively small single team. As I said however, I could require review from specific people I know review but they're already at their wits end. Also having to explain why certain devs are required without it smelling like some sort of favoritism seems fraught.
    - Aeolun 2y ago
      Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?
- Kinrany 2y ago
  Anything you do to make people fix their 3000 loc PRs will be unpopular with those people
dukedylan 2y ago
I'm shocked how many companies still don't focus on DevX and wonder why not only is productivity in the gutter, but morale as well. There's so many companies that help solve DevX problems like this, that's it's practically easy to throw money at it.
- MortyWaves 2y ago
  I ask myself this often, usually while despairing that the lead dev doesn’t see any point to build pipelines, unit tests, SQL migrations (literally right clicks and edits a database schema), and the requirement of needing Remote Desktop to a VM to do anything.
random3 2y ago
It's never the process it's always the people and the underlying competence gap that kills morale.
declan_roberts 2y ago
200 comments mostly about stylistic nitpicking is actually a language fault, not a team fault. Languages like Golang are specifically designed to remove any stylistic nitpicking. Every language must follow. It's suicidal otherwise.
agentultra 2y ago
Yes! I’d also add that it’s helpful to have a review guideline. When I get to the PR phase, I’ve already done the design work, considered the trade-offs, and am asking folks to check that the code is mergeable per our guidelines. A pet peeve of mine are reviewers with a bone to pick who will leaving blocking comments to redo the work in the way they would have approached it. A guideline helps here because you can, along with your team, manage expectations for the process. Some folks like to throw up code for critiques. They use PRs to get feedback on the design/approach itself. That can be useful! Going in to manage expectations helps smooth things along so that you’re giving the constructive feedback the author is looking for.
stared 2y ago
One core issue is that PRs are often too large. I don't believe there are 300 comments on a small PR unless the team is dysfunctional. Aim to make them as atomic as possible. If it involves adding a significant feature that requires substantial changes to other modules, create a separate branch for it. Within this branch, break the work down into a series of smaller PRs. When PRs are too large, the process usually suffers from insufficient reviews, excessive nitpicking, or (most often) a combination of both.
bsnnkv 2y ago
I can't imagine going back to working on codebases in languages that don't come with strongly opinionated defaults. I'm very lucky to spend the majority of my dayjob hours working on Rust code, so everyone just runs cargo fmt && cargo clippy, and this in enforced in CI. You can't even publish a PR until those basic bars have been met. I can't imagine the absolute insanity of working on JS projects where there are more ways to do things than there are people on a team, and where getting those people with strongly held opinions about things that ultimately just don't matter to agree on style and convention is like pulling teeth.
- thiht 2y ago
  JS has excellent tooling for standardizing code style, Prettier for opinionated formatting and ESLint for everything else. If some teams decide to not use these tools, that’s their mistake.
  - Aeolun 2y ago
    Or Biome for both. And in 1 second instead of 10 minutes. Sure, eslint has a few extra rules, but in my opinion it just doesn’t beat getting results at sub second speeds.
pyrale 2y ago
I wholly agree with this, and I would add worthless Sonar opinion reports th the list.
praptak 2y ago
Combine this with a large timezone difference and you get a (literally) slow motion disaster. Imagine waking up and finding your change thoroughly reviewed but not approved because of a triviality. Not a great day.
- hermanradtke 2y ago
  My bias is towards merging. Trivial issues can be fixed in a future commit or PR.
  - mckn1ght 2y ago
    This is my life right now and it is terrible, can confirm. I’ve tried advocating for what you say to no avail. Or like, just opening a new PR into my branch, or directly committing to my branch. I’m not territorial, I just want progress. Instead people love to bicker and cover their own ass.
lkrubner 2y ago
When it comes to code reviews, the return on investment faces the Law Of Diminishing Returns. While many of the comments made in code reviews might be interesting, they are not so interesting that they pay for themselves. If you put some dollar value on the time invested, you'll find that the vast majority of this process is simply burning money. And not only money, but also, as this article says, morale. A curious fact about the politics of modern tech teams is that some of the same people who consider themselves "anti bureaucracy" are strongly in favor of this type of bureaucracy, even when it brings no measurable benefits. My opinion of code reviews has become more negative over time, mostly due to the fact that I have not seen it achieve reliable positive outcomes. I'll give specific examples: https://www.futurestay.com https://www.futurestay.com had the worst code that I'd ever seen. When I joined the company I was horrified at the level of tech debt. And yet, for years, they had a rule that at least two engineers had to review every PR. So this terrible code, the worst I'd seen in my 24 years of coding, had been approved by two engineers. And likewise: https://openroadmedia.com https://openroadmedia.com also had very bad code, and also had a rule that nothing could be pushed to production until at least 2 engineers had reviewed the PR. In both companies the code review slowed down the deployments, slowed the down the team, and created a culture of internal sniping, while leading to no improvements. But how can I say the code was objectively bad? Because many bugs were found in production. And what would reduced the number of bugs in production? More tests, especially end-to-end tests. And so I eventually came to this conclusion: it is best to skip code review, and instead have the team invest that same time in writing more tests, especially high level tests. For the most part, if the engineers on a team are bad then the code reviews will also be bad, but if the engineers on the team are good, the the code reviews will not be needed. And in both cases, the path to improvement comes from writing the kinds of tests that ensure no bugs get into production. As I've grown in my career, and taken on higher level management jobs, I've also realized that code reviews do not scale to large-scale leadership, but tests do. Code review seemed like a good idea when I was leading a team of 3 engineers, but not when I was leading a team of 30. When I was leading a team of 30, the only tool-of-oversight that worked for me was automated testing. And so I've concluded this is where the effort should be made. Sitting at my own computer, I cannot review the work of 30 engineers, but I can still run the tests on the various software, to see if they are passing. In particular, I can look at API interfaces and then change the dummy test data to break the tests, and this lets me see quickly thorough the test coverage is, and how prepared we are for unexpected shocks. When I am running a team I will assign software developers the task of writing various tests, including high-level end-to-end stress tests. These tests sometimes reveal problems. We then respond to the problems. But we don't waste time responding to problems that have not been proven by a test, which is to say we do not do code reviews. Code reviews are all about predicting what might be a problem. But much of the concerns are phantoms. It is better to respond to real problems that have been revealed by tests. I've also come to realize that software developers, quite naturally, develop strong opinions about questions of style, and yet these questions have no long-term impact on the health of the tech in the company. Other than enforcing the rules of some automated linter, the time spent on issues of style are 100% wasted. If you allow the team to spend even one minute discussing issues of style, then you are setting money on fire. But I know this opinion is unpopular, because software developers enjoy enforcing their own opinions about style. But it is all bikeshedding, it has no real-world impact. I have the impression that there is some middle era, in the career of a software developer, where concerns about issues of style and organization tend to peak. The junior developer does not know enough to care about such issues, but somewhere between 4 years of experience and 10 years of experience, these issues feel important. I think you need more than 10 years of experience to see the waste. In particular, you need to run one large project where the team invests a lot of time into code reviews, and you need to notice how much tech debt builds up, despite the code reviews, to realize that code reviews do not offer a path forward. On recent projects I've told the team there will be no code reviews, but instead we will focus on building tests. I get a surprising amount of pushback on this. Less experienced software developers get angry with me and tell me that I'm being unprofessional. In some sense they are correct, in the sense that "professional" refers to "standard norms that are accepted by a profession" -- I am deviating from those, clearly. The most reasonable criticism I get is that code reviews could catch not just problems of style but problems of algorithms. What if, they ask, some junior developer introduces code that is ignorant of the implications of Big O Notation? What if they introduce code that runs in polynomial time? But I would ask, how do we know if it is running slowly? We can only know that through tests. So lets build tests that measure time, and stress test with large loads. Big O Notation offers an excellent example of where software developers can worry about the wrong thing: what if a junior level software developer writes code that runs in polynomial time but on data that will only ever have a few hundred records? While the algorithm might be sloppy, the code will run quickly because there are few records? In that case, any time invested to find a different algorithm will be a poor investment. Once I'm running a team of 30, the only thing I care about is whether code is actually slow, and I discover that through end-to-end stress tests, not code reviews. Kent Beck, when he invented Xtreme Programming, also popularized the phrase "Do not write a comment, instead, write a method with an easy-to-understand name that communicates what the comment was going to communicate." I've come to a similar conclusion. Do not write a comment in a code review, instead, write a test that would catch whatever danger you want to warn about. And if you cannot find a way to express your concern as a test, the return-on-investment of worrying about that concern is probably zero, so we should ignore it.
hnlurker22 2y ago
I worked at a startup where a developer used to bully other developers in PRs. Management looked away. I warned them but they didn't listen. That team is now dissolved.
RATANKUMA 2y ago
[flagged]
rileymat2 2y ago
Part of me wonders if the real issue is we don't have an author/editor type system. If the reviewer/editor could just make the nitpicky changes in about the same time it takes to call them out, the relationship might be much more healthy. Things could go back and forth in a much more healthy way.
- t-writescode 2y ago
  Jetbrains SpaceCode (rip), Github and Gitlab all provide that sort of system. I imagine the other ones do, too.
  - rileymat2 2y ago
    Sorry, I was not clear. I don’t mean software system, but a culture of operating in that way. Even the language “Code Review” is not one of cooperation, but creates thoughts of a “movie review” where one rates and finds error; not collaboration and mutual editing.
- tremon 2y ago
  We do, in a way, but it's not recognized as such. The "author" would be the mythical 10x developer that races to get feature tickets completed, the editors are the ones getting stuck with integrating a pile of stinking hotness into the existing codebase. That kind of setup can work fine if everyone is aware of the others' value, but usually non-technical managers only see the hotness.
powersnail 2y ago
I don't know the original story, but 300 hundred PR comments on a first PR (or any PR for that matter) is insane, and my guess is that somebody on the team is having an argument back-and-forth in that PR, rather than focusing on whether the code should be merged. If that happens to me, I would kindly invite them to talk about it elsewhere and give me pass, because the fact that you guys are still debating it, means that it's not a decided thing, and you can't prosecute me with a law that hasn't passed yet. Once it's actually settled, I'll be happy to go back and refactor it, but it is illogical to make it a blocker in the present. It also sounds like something is wrong with the on-boarding process. The first PR should not be of such a nature that would invite loads of comments. It should be something that is functionally simple, and already has an agreed-upon design.
- steveBK123 2y ago
  I was once on a team where this was not unusual, and the problem was the tech lead was the one engaging in 300 PR comment arguments. This would result in stuff sitting in purgatory forever. The funniest part was this was, in 20 years, the absolute worst codebase I'd ever seen, so he wasn't even maintaining some high standards in the codebase by doing so.
  - powersnail 2y ago
    Sounds about right. If so many technical decisions are made by a single individual while reviewing PR, rather than having a proper standard agreed upon before hand, it's no wonder that it grows into a horrible code base.
    - steveBK123 2y ago
      I think PRs are also the wrong place to litigate any of this. Requirements, specs, architecture, etc sure. But most systems I've worked on that were especially BAD were that way not because of some PR-reviewable syntax or style, but because the whole foundation was wrong. The most idiomatic, consistent, linted, unit-tested, etc code in the world doesn't matter that much if the underlying architecture is a mess.
nmstoker 2y ago
This is really important to get right. There's a balance as you want to avoid introducing crap but a little restraint and wider awareness makes a huge difference. I recall being significantly put off after making my first significant contribution online to a non-trivial repo. I was a volunteer and had been making various very modest contributions (ie simple fixes) along with efforts around docs and answering questions from users. A major contributor was known for being cantankerous. He knew what I was bringing in general, and seemed grateful yet didn't hold off with pretty forceful feedback on my first attempt at something more substantial - I had taken a day off to get it in good shape, I knew it was solving something with broad use (ie not just for me!). There were a litany of points. I fixed several but each time there would be more, it was like water on Gremlins! I gave up and stood back for a bit. Ironically it would have been trivial for him to take the code in but it sat forever and the whole thing was a waste of time but I guess I learned a few things! I try to encourage standards but not officiously!
patrickhogan1 2y ago
I’ve had a PR into an open source Anthropic library open for almost 2 months. It passes all of their rules, fixes an obvious bug that causes functional issues, passes all tests, and has a description that is descriptive with a complete user story explaining the functional issue with images. I really like using Claude and I like Anthropic’s mission but it definitely kills morale to fix something and be put into PR purgatory.
cyrkularsaw 2y ago
[dead]
jibbit 2y ago
People being an obstacle to positive change, because they believe a better change is possible.. when the changes wouldn't in any way prevent further positive changes from happening.. are the bane of my life. 'bikeshedding' probably covers it, but i feel like if someone could come up with a better metaphor that wasn't specifically about triviality or level of comprehension it could be a profound contribution to human behaviour
tired_and_awake 2y ago
If you join a company and are confronted with dozens of PR comments - assuming this process isn't sufficiently well documented - see if a tech lead or manager will host an open forum review. Review the code and the comments and discuss + document. A one off discussion won't be a permanent fix, but it can help!
sibit 2y ago
I have always found articles/discussions of code reviews fascinating. In the 10(ish) years I've been employed as a programmer I've never worked in a department with more than ten people and every time the work is distributed such that each person works as an IC on their own siloed project(s). Receiving anything besides the "LGTM" rubber stamp or nitpick comment seems almost impossible.
whynotmaybe 2y ago
> Define (and Stick To) a Style Guide We have some styling rules that are so much in the "Style Over Substance" category that I have a strong tendency to subconsciously categorize them as "not important, at all". Especially when the IDE doesn't respect them when you use the code generation. But as it was a strong irritant for some PR reviewers, and as I couldn't force my mind stop doing it, I automated it by writing a custom linter.
anothername12 2y ago
I have quit places because of their PR process.
mshekow 2y ago
I found two ideas / techniques helpful in this context: 1) Conventional comments (https://conventionalcomments.org/ https://conventionalcomments.org/) as an (agreed-upon) language to be used in PR comments 2) Ship / Show / Ask (https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html), where "Show" and "Ship" are non-blocking PRs (or even directly committing to trunk, if you use trunk-based development), since not every(!) PR needs reviewing and/or should block the PR creator
ilaksh 2y ago
The only thing I disagree with him on this is that function length doesn't matter. Longer functions do tend to be harder to understand. I will admit that sometimes a lot of helper functions can make it harder to piece together, so you can overdo it, but usually putting one or two of them back into the main function gives a very readable function. So I would tend towards more smaller more decomposed functions with descriptive (but concise as possible) names by default. This is very different from old-fashioned C standards, which I feel are very outdated for modern tooling and hardware. It's definitely important to have formatting standards to avoid arguments about that. But you are never going to completely eliminate stylistic things because part of that is function decomposition or intersects with the actual substantive design. Because some people feel quite differently about function size for example.
Aeolun 2y ago
Where do people find these places? I have a hard time getting management to accept that more restrictions on who can review is not a bad thing. Nearly 90% of our PR’s have zero comments. I do have a hard ban on formatting comments though. If it’s not covered by the linter/formatter everything goes.
chromanoid 2y ago
Non blocking code reviews ftw https://itnext.io/optimizing-the-software-development-process-for-continuous-integration-and-flow-of-work-56cf614b3f59 https://itnext.io/optimizing-the-software-development-proces... Optimizing the Software development process for continuous integration and flow of work https://medium.com/itnext/how-feature-branches-and-pull-requests-work-against-best-practice-a13a85a016ef https://medium.com/itnext/how-feature-branches-and-pull-requ... How feature branches and pull requests work against best practice
swisniewski 2y ago
I usually adopt a policy with my teams: You are not allowed to complain about style in code reviews. If it’s important enough for your to comment about, it’s important enough for you to add a linter to the build and block the build if the style isn’t met. If it’s not worth enough of your time to add a linter, it’s certainly not worth your peer’s time to deal with comments about style.
scotty79 2y ago
Code reviews are weird. I joined a very small team as a new developer. I got a task, solved it but at code review the senior developer told me to solve it in another specific way. I coded it and it was fine. The situation repeated for few next tasks. It was a bit frustrating because I felt like doing the job twice while "pre-review" of my tasks by the ultimate code reviewer before I started doing them could save both my time and his. But it just wasn't part of the process. Does such thing have a name? A quick check up with a reviewer about whether my plan for implementing the task is align with their preferences?
deleted 2y ago
[deleted]
Andy101 2y ago
[dead]

Per the article, not the pull requests itself, but the tedious bikeshedding around them. The article's headline is a bit misleading.

rob74 2y ago
That's probably what they mean by "PR Process"?
- WesolyKubeczek 2y ago
  "Your" got editorialized away and the title became quite categorical. Implying all of them kill morale and are flawed.

DragonStrength 2y ago

Watched a guy get fired mostly because he made code review such a nightmare for everyone else while getting lapped by everyone in terms of actual output. I’m not sure there’s much to be done once you’ve hit “toxic.” The only question is whether the standard can be reset for the whole team.

treespace8 2y ago

GIT seams to be optimized for network of trust. With one person at the top approving what gets merged into the release. This person of course does not do all of the verification, other then broad strokes of what the change does, and who wrote it, reviewed it and tested it. I feel like companies do not want a large tree like structure for their development teams. Without a network of trust it can become mob rule, which is what this article appears to be describing.

yjftsjthsd-h 2y ago
That makes sense; https://en.wikipedia.org/wiki/Conway%27s_law https://en.wikipedia.org/wiki/Conway%27s_law applies and git was created by Linus Torvalds for Linux, which works exactly like that.

barbazoo 2y ago

> I’ve recently come across a discussion where a new developer joined a team and faced over 300 PR comments on their first contribution. Most of it was stylistic nitpicking. This isn’t just unproductive, it’s outright toxic. For me this says more about the company culture than any inherent flaws with the code review process.

znpy 2y ago
on a different side: it also tell you (a lot) about specific people. I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter). On the other hand bad/toxic people will drown you with stylistic nitpicks and won't approve (and trust) you to do your best work. You will be essentially blocked pending their approval (so that nitpicks are changed according to their likings). The weird thing is that all this traceability leaves traces for management to see who's doing a good pr review job and who's not... But I've learned that management usually does not care much.
- prh8 2y ago
  "will approve after nitpicks" is the most asinine behavior
- barbazoo 2y ago
  Agree. Why did the junior developer feel like their changes were ready to be reviewed. Looks like they had little guidance. Why didn't a senior developer suggest to close the PR, apply the formatting or whatever and then re-open the PR or something like that. 300 comments, that's either a lot of developers commenting or just a lot of back and forth. I don't get how anyone would let it come to that point. Makes me feel grateful for the places I worked at and the experience I have now.
- dec0dedab0de 2y ago
  * I've seen good/great people call out the nitpicks (in my case it was often mis-spelling, due to not being a native speaker of english) but will approve the PR anyway (implicitly expecting another revision to be sent, trusting the submitter).* I always thought this was the best way. I wish these systems had a way to assign severity to comments, and urgency to the commit. If you have a jr developer it is your job to give them stylistic feedback, the problem comes from mixing it in with security holes or sneaky bugs. And when the process doesn’t identify when we need to ship it yesterday, vs in the next few months.

prh8 2y ago

I've worked with plenty of devs who embody the toxicity here, lots of stylistic nitpicks and don't see the big picture. All the noise generated distracts from catching the important stuff.

Joker_vD 2y ago

...just how big was that "first ever contribution" pull request that it gathered 300 comments? My first commit in the current company was a 2-liner addition to an existing function plus another 20 lines for a test that verified that indeed, the change does affect the outcome.

barbazoo 2y ago
Even a simple (and very appropriate for a junior/newbie) change like you suggested could invite dozens of comments if the engineers don't know what they're doing.
- Joker_vD 2y ago
  "Dozens" means "at least 24", so that'd been slightly more than 2 comments per line in my example. Which is an insane amount of comments by any standard imaginable: because long before you reach 1-1 comments-to-code ratio, you'd stop, and write a single "no, all of this is a wholly incorrect approach, complete re-do is needed" comment instead.
  - mckn1ght 2y ago
    You’re right, but some people out there will flex and grandstand in their reviews if they operate adversarially instead of collaboratively, which is what your suggestion would encourage.

zie1ony 2y ago

Recently I allowed everyone in my team to push to develop without PRs and even without doing feature branches. We review the code all together (max 2 ppl) just before the release and that's it. But great CI pipeline is must for this. It will get even better soon when AI will be able to slightly refactor the code.

lapcat 2y ago
> We review the code all together (max 2 ppl) just before the release and that's it. Is there a reason why you don't review the code immediately? You could also do that without PRs.
- pavel_lishin 2y ago
  Yeah, I've heard arguments for this, and it always feels weird to me. Why go so long without review? That's just giving developers - junior and senior - opportunity to dig themselves into a hole that they don't notice until someone points out, "hey, your last five commits are buggy, but that matters less than the fact that they're implementing the wrong thing."
  - zie1ony 2y ago
    Good point, but my team is small and has only top engineers with both soft and hard skills. Many times you can do much more with team of 3 than team of 10 ppl.
    - Aeolun 2y ago
      If the three sit next to one another I agree that PR’s are overkill. If they’re remote it becomes harder I think.

BurningFrog 2y ago

Another angle is that a PR with over 300 comments is probably way too big. Many small focused PRs is IMAO much better than big PRs with weeks or months of work in them. But that requires fast and friendly PR reviews!

eddd-ddde 2y ago
It also requires good tooling and some experience. GitHub absolutely sucks at making multi-commit features. At least last time I used it. And getting people to actually make smaller commits and reviews is incredibly hard.
- herpdyderp 2y ago
  Can confirm GitHub's process is poor for small PRs (if that's what you mean by "multi-commit features"). My team has mostly gotten around this with custom CLI tools (for pushing small PRs chained together) and web apps (for concisely viewing your code review status, both giving and receiving).
  - kleinsch 2y ago
    I’m a huge fan of https://github.com/VirtusLab/git-machete https://github.com/VirtusLab/git-machete
    - mckn1ght 2y ago
      Looks great, like a combination of the gh cli and the many options available for maintaining chains of stacked branches!

gboss 2y ago

Whenever I encounter a pull request that I find many issues with, I ask to meet with the engineer and review it one on one. More than half of the workplace problems engineers have is due to their introverted nature and their refusal to get on a teams/zoom call to explain their issues and get resolution. Comments are a really poor mechanism for teaching programming best practices.

BurningFrog 2y ago
At my last job, the rule was if the PR is big enough, get on a call and go over it in person. ideally with more than one "reviewer", to break stalemates. Worked really well! I used to be that introverted guy, but a few years of pair programming completely cured that, and I'm now very comfortable discussing design in detail and at length, in a (if I may say so myself) friendly and constructive way. If you've never discussed design with other people, it's a genuinely difficult thing to do!
- skydhash 2y ago
  A PR big enough should probably have its changes implemented and reviewed unit by unit on a feature branch before its creation. Or it should be an already approved work (refactoring, reformatting,...)
  - t-writescode 2y ago
    And if it's refactoring / reformatting, then it should be pair-programmed to validate that exactly what was said to have been done was done and the pair programmer should confirm such in the PR.
- mckn1ght 2y ago
  I would love this approach, but the people reviewing my PRs are 10 tzs ahead of me, so we basically can’t meet synchronously unless someone is willing to meet early in the morning or late in the evening. Maybe prerecorded code walkthroughs would help here, but nothing replaces instant face to face pairing. That’s where you can provide somewhat nitpicky feedback but with the empathy that comes with our instinctual responses to body language and voice tone… which over time builds a much better culture IMO.

barbazoo 2y ago

I recommend using something like https://github.com/erikthedeveloper/code-review-emoji-guide https://github.com/erikthedeveloper/code-review-emoji-guide in code reviews to signify what your expectation is exactly.

SoKamil 2y ago
Can you share your experience with this kind of review process?
- barbazoo 2y ago
  I can mirror what their docs say > Using CREG (Code Review Emoji Guide) puts more ownership on the reviewer to give the reviewee added context and clarity to follow up on code review. For example, knowing whether something really requires action (), highlighting nit-picky comments (), flagging out of scope items for follow-up () and clarifying items that don’t necessarily require action but are worth saying ( , , )

jchw 2y ago

Personally I have a strong distaste for projects that try to use some metric for how long a function should be, e.g. line count or cyclomatic complexity. I'm not sure if this one is better automated, human judgement for what makes sense seems better to me. Sometimes the cleanest and highest performance way to write some code is going to be basically one relatively large function. If there's a function complexity/size lint I'll often chunk off helper functions that are virtually useless outside of that specific function, and in doing so, often make it harder to follow what's going on. This isn't universally true of course, but it's a "when not if" situation if you have a hard lint limit and it's not very high. (e.g. In my opinion if you're going to have a lint for this, set it to something rather obscene, something that would be extremely hard to cross without doing something clearly awful. In my opinion it's always been set at least 4 times lower than it should be.)

Kinrany 2y ago
It shouldn't be a hard rule, but in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context. All a linter does automatically is require adding the "ignore this line" comments that are then visible to humans in diffs.
- bryanlarsen 2y ago
  A linter directive also tells humans that somebody has decided that the function being long is acceptable in this specific location. It's been acknowledged. In many cases, that's all I want.
  - mckn1ght 2y ago
    Then you go from pre-linter arguments about line/function/class length to arguments over whether it’s appropriate to override the linter rule in those same instances.
- bsder 2y ago
  > in most cases a sufficiently long function will have opportunities for pulling out functions with sensible signatures that make sense outside that context. And will make understanding the function a lot more difficult. If you want an example of this, look at any C++ code that uses DX12 or Vulkan. It will be atomized sufficiently that you'll have to hunt through dozens of files to figure out which constructor/destructor where something fired. Part of the problem is that C++ discourages naked functions operating on structs rather than member functions on classes. Whereas, if those functions were still all linear in a single function, you'd find what you were looking for in the same file you were already in. That having been said, I'll probably organize that big function as though it were separate functions. But, then, I'm not allergic to adding an extra block level here or there which GASP might exceed 80 columns.

donatj 2y ago

My team has a bike shedding sort of problem where a 100 loc PR will sometimes get scrutinized to hell, but a 3,000 loc PR will get LGTM'd by enough of the team to be merged before anyone that actually cares gets a chance to look at it. I would say the second half of that is the much bigger problem. People know who to ask to get a quick lgtm. I don't know what to do about it. I can't make people actually review. I've had unrealistic dreams like holding people responsible for things they approved that had bugs, but anything like that would be supremely unpopular. I could do something like requiring review from the team members that actually review, but they already feel overwhelmed by being the only ones that actually review. We tried setting soft limits on the size of PRs, but that comes with a lot of PR that are hard to review because the work is poorly divided and doesn't make sense in isolation.

simmschi 2y ago
Have you tried the code owners feature (assuming you're on Github). IMO a good approach is to have the actual code owners (i.e. the team responsible for a specific service or library) review the PR. If they think a shallow LGTM review of 3k LOC is enough, they can also deal with the bugs :-) If you don't have specific ownership in your code base I'd start there.
- donatj 2y ago
  This is all within a relatively small single team. As I said however, I could require review from specific people I know review but they're already at their wits end. Also having to explain why certain devs are required without it smelling like some sort of favoritism seems fraught.
  - Aeolun 2y ago
    Normally you promote those people to ‘senior’ and then say that a review by at least one senior is required?
Kinrany 2y ago
Anything you do to make people fix their 3000 loc PRs will be unpopular with those people

dukedylan 2y ago

I'm shocked how many companies still don't focus on DevX and wonder why not only is productivity in the gutter, but morale as well. There's so many companies that help solve DevX problems like this, that's it's practically easy to throw money at it.

MortyWaves 2y ago
I ask myself this often, usually while despairing that the lead dev doesn’t see any point to build pipelines, unit tests, SQL migrations (literally right clicks and edits a database schema), and the requirement of needing Remote Desktop to a VM to do anything.

random3 2y ago

It's never the process it's always the people and the underlying competence gap that kills morale.

declan_roberts 2y ago

200 comments mostly about stylistic nitpicking is actually a language fault, not a team fault. Languages like Golang are specifically designed to remove any stylistic nitpicking. Every language must follow. It's suicidal otherwise.

agentultra 2y ago

Yes! I’d also add that it’s helpful to have a review guideline. When I get to the PR phase, I’ve already done the design work, considered the trade-offs, and am asking folks to check that the code is mergeable per our guidelines. A pet peeve of mine are reviewers with a bone to pick who will leaving blocking comments to redo the work in the way they would have approached it. A guideline helps here because you can, along with your team, manage expectations for the process. Some folks like to throw up code for critiques. They use PRs to get feedback on the design/approach itself. That can be useful! Going in to manage expectations helps smooth things along so that you’re giving the constructive feedback the author is looking for.

stared 2y ago

One core issue is that PRs are often too large. I don't believe there are 300 comments on a small PR unless the team is dysfunctional. Aim to make them as atomic as possible. If it involves adding a significant feature that requires substantial changes to other modules, create a separate branch for it. Within this branch, break the work down into a series of smaller PRs. When PRs are too large, the process usually suffers from insufficient reviews, excessive nitpicking, or (most often) a combination of both.

bsnnkv 2y ago

I can't imagine going back to working on codebases in languages that don't come with strongly opinionated defaults. I'm very lucky to spend the majority of my dayjob hours working on Rust code, so everyone just runs cargo fmt && cargo clippy, and this in enforced in CI. You can't even publish a PR until those basic bars have been met. I can't imagine the absolute insanity of working on JS projects where there are more ways to do things than there are people on a team, and where getting those people with strongly held opinions about things that ultimately just don't matter to agree on style and convention is like pulling teeth.

thiht 2y ago
JS has excellent tooling for standardizing code style, Prettier for opinionated formatting and ESLint for everything else. If some teams decide to not use these tools, that’s their mistake.
- Aeolun 2y ago
  Or Biome for both. And in 1 second instead of 10 minutes. Sure, eslint has a few extra rules, but in my opinion it just doesn’t beat getting results at sub second speeds.

pyrale 2y ago

I wholly agree with this, and I would add worthless Sonar opinion reports th the list.

praptak 2y ago

Combine this with a large timezone difference and you get a (literally) slow motion disaster. Imagine waking up and finding your change thoroughly reviewed but not approved because of a triviality. Not a great day.

hermanradtke 2y ago
My bias is towards merging. Trivial issues can be fixed in a future commit or PR.
- mckn1ght 2y ago
  This is my life right now and it is terrible, can confirm. I’ve tried advocating for what you say to no avail. Or like, just opening a new PR into my branch, or directly committing to my branch. I’m not territorial, I just want progress. Instead people love to bicker and cover their own ass.

lkrubner 2y ago

When it comes to code reviews, the return on investment faces the Law Of Diminishing Returns. While many of the comments made in code reviews might be interesting, they are not so interesting that they pay for themselves. If you put some dollar value on the time invested, you'll find that the vast majority of this process is simply burning money. And not only money, but also, as this article says, morale. A curious fact about the politics of modern tech teams is that some of the same people who consider themselves "anti bureaucracy" are strongly in favor of this type of bureaucracy, even when it brings no measurable benefits. My opinion of code reviews has become more negative over time, mostly due to the fact that I have not seen it achieve reliable positive outcomes. I'll give specific examples: https://www.futurestay.com https://www.futurestay.com had the worst code that I'd ever seen. When I joined the company I was horrified at the level of tech debt. And yet, for years, they had a rule that at least two engineers had to review every PR. So this terrible code, the worst I'd seen in my 24 years of coding, had been approved by two engineers. And likewise: https://openroadmedia.com https://openroadmedia.com also had very bad code, and also had a rule that nothing could be pushed to production until at least 2 engineers had reviewed the PR. In both companies the code review slowed down the deployments, slowed the down the team, and created a culture of internal sniping, while leading to no improvements. But how can I say the code was objectively bad? Because many bugs were found in production. And what would reduced the number of bugs in production? More tests, especially end-to-end tests. And so I eventually came to this conclusion: it is best to skip code review, and instead have the team invest that same time in writing more tests, especially high level tests. For the most part, if the engineers on a team are bad then the code reviews will also be bad, but if the engineers on the team are good, the the code reviews will not be needed. And in both cases, the path to improvement comes from writing the kinds of tests that ensure no bugs get into production. As I've grown in my career, and taken on higher level management jobs, I've also realized that code reviews do not scale to large-scale leadership, but tests do. Code review seemed like a good idea when I was leading a team of 3 engineers, but not when I was leading a team of 30. When I was leading a team of 30, the only tool-of-oversight that worked for me was automated testing. And so I've concluded this is where the effort should be made. Sitting at my own computer, I cannot review the work of 30 engineers, but I can still run the tests on the various software, to see if they are passing. In particular, I can look at API interfaces and then change the dummy test data to break the tests, and this lets me see quickly thorough the test coverage is, and how prepared we are for unexpected shocks. When I am running a team I will assign software developers the task of writing various tests, including high-level end-to-end stress tests. These tests sometimes reveal problems. We then respond to the problems. But we don't waste time responding to problems that have not been proven by a test, which is to say we do not do code reviews. Code reviews are all about predicting what might be a problem. But much of the concerns are phantoms. It is better to respond to real problems that have been revealed by tests. I've also come to realize that software developers, quite naturally, develop strong opinions about questions of style, and yet these questions have no long-term impact on the health of the tech in the company. Other than enforcing the rules of some automated linter, the time spent on issues of style are 100% wasted. If you allow the team to spend even one minute discussing issues of style, then you are setting money on fire. But I know this opinion is unpopular, because software developers enjoy enforcing their own opinions about style. But it is all bikeshedding, it has no real-world impact. I have the impression that there is some middle era, in the career of a software developer, where concerns about issues of style and organization tend to peak. The junior developer does not know enough to care about such issues, but somewhere between 4 years of experience and 10 years of experience, these issues feel important. I think you need more than 10 years of experience to see the waste. In particular, you need to run one large project where the team invests a lot of time into code reviews, and you need to notice how much tech debt builds up, despite the code reviews, to realize that code reviews do not offer a path forward. On recent projects I've told the team there will be no code reviews, but instead we will focus on building tests. I get a surprising amount of pushback on this. Less experienced software developers get angry with me and tell me that I'm being unprofessional. In some sense they are correct, in the sense that "professional" refers to "standard norms that are accepted by a profession" -- I am deviating from those, clearly. The most reasonable criticism I get is that code reviews could catch not just problems of style but problems of algorithms. What if, they ask, some junior developer introduces code that is ignorant of the implications of Big O Notation? What if they introduce code that runs in polynomial time? But I would ask, how do we know if it is running slowly? We can only know that through tests. So lets build tests that measure time, and stress test with large loads. Big O Notation offers an excellent example of where software developers can worry about the wrong thing: what if a junior level software developer writes code that runs in polynomial time but on data that will only ever have a few hundred records? While the algorithm might be sloppy, the code will run quickly because there are few records? In that case, any time invested to find a different algorithm will be a poor investment. Once I'm running a team of 30, the only thing I care about is whether code is actually slow, and I discover that through end-to-end stress tests, not code reviews. Kent Beck, when he invented Xtreme Programming, also popularized the phrase "Do not write a comment, instead, write a method with an easy-to-understand name that communicates what the comment was going to communicate." I've come to a similar conclusion. Do not write a comment in a code review, instead, write a test that would catch whatever danger you want to warn about. And if you cannot find a way to express your concern as a test, the return-on-investment of worrying about that concern is probably zero, so we should ignore it.

hnlurker22 2y ago

I worked at a startup where a developer used to bully other developers in PRs. Management looked away. I warned them but they didn't listen. That team is now dissolved.

RATANKUMA 2y ago

[flagged]

rileymat2 2y ago

Part of me wonders if the real issue is we don't have an author/editor type system. If the reviewer/editor could just make the nitpicky changes in about the same time it takes to call them out, the relationship might be much more healthy. Things could go back and forth in a much more healthy way.

t-writescode 2y ago
Jetbrains SpaceCode (rip), Github and Gitlab all provide that sort of system. I imagine the other ones do, too.
- rileymat2 2y ago
  Sorry, I was not clear. I don’t mean software system, but a culture of operating in that way. Even the language “Code Review” is not one of cooperation, but creates thoughts of a “movie review” where one rates and finds error; not collaboration and mutual editing.
tremon 2y ago
We do, in a way, but it's not recognized as such. The "author" would be the mythical 10x developer that races to get feature tickets completed, the editors are the ones getting stuck with integrating a pile of stinking hotness into the existing codebase. That kind of setup can work fine if everyone is aware of the others' value, but usually non-technical managers only see the hotness.

powersnail 2y ago

I don't know the original story, but 300 hundred PR comments on a first PR (or any PR for that matter) is insane, and my guess is that somebody on the team is having an argument back-and-forth in that PR, rather than focusing on whether the code should be merged. If that happens to me, I would kindly invite them to talk about it elsewhere and give me pass, because the fact that you guys are still debating it, means that it's not a decided thing, and you can't prosecute me with a law that hasn't passed yet. Once it's actually settled, I'll be happy to go back and refactor it, but it is illogical to make it a blocker in the present. It also sounds like something is wrong with the on-boarding process. The first PR should not be of such a nature that would invite loads of comments. It should be something that is functionally simple, and already has an agreed-upon design.

steveBK123 2y ago
I was once on a team where this was not unusual, and the problem was the tech lead was the one engaging in 300 PR comment arguments. This would result in stuff sitting in purgatory forever. The funniest part was this was, in 20 years, the absolute worst codebase I'd ever seen, so he wasn't even maintaining some high standards in the codebase by doing so.
- powersnail 2y ago
  Sounds about right. If so many technical decisions are made by a single individual while reviewing PR, rather than having a proper standard agreed upon before hand, it's no wonder that it grows into a horrible code base.
  - steveBK123 2y ago
    I think PRs are also the wrong place to litigate any of this. Requirements, specs, architecture, etc sure. But most systems I've worked on that were especially BAD were that way not because of some PR-reviewable syntax or style, but because the whole foundation was wrong. The most idiomatic, consistent, linted, unit-tested, etc code in the world doesn't matter that much if the underlying architecture is a mess.

nmstoker 2y ago

This is really important to get right. There's a balance as you want to avoid introducing crap but a little restraint and wider awareness makes a huge difference. I recall being significantly put off after making my first significant contribution online to a non-trivial repo. I was a volunteer and had been making various very modest contributions (ie simple fixes) along with efforts around docs and answering questions from users. A major contributor was known for being cantankerous. He knew what I was bringing in general, and seemed grateful yet didn't hold off with pretty forceful feedback on my first attempt at something more substantial - I had taken a day off to get it in good shape, I knew it was solving something with broad use (ie not just for me!). There were a litany of points. I fixed several but each time there would be more, it was like water on Gremlins! I gave up and stood back for a bit. Ironically it would have been trivial for him to take the code in but it sat forever and the whole thing was a waste of time but I guess I learned a few things! I try to encourage standards but not officiously!

patrickhogan1 2y ago

I’ve had a PR into an open source Anthropic library open for almost 2 months. It passes all of their rules, fixes an obvious bug that causes functional issues, passes all tests, and has a description that is descriptive with a complete user story explaining the functional issue with images. I really like using Claude and I like Anthropic’s mission but it definitely kills morale to fix something and be put into PR purgatory.

cyrkularsaw 2y ago

[dead]

jibbit 2y ago

People being an obstacle to positive change, because they believe a better change is possible.. when the changes wouldn't in any way prevent further positive changes from happening.. are the bane of my life. 'bikeshedding' probably covers it, but i feel like if someone could come up with a better metaphor that wasn't specifically about triviality or level of comprehension it could be a profound contribution to human behaviour

tired_and_awake 2y ago

If you join a company and are confronted with dozens of PR comments - assuming this process isn't sufficiently well documented - see if a tech lead or manager will host an open forum review. Review the code and the comments and discuss + document. A one off discussion won't be a permanent fix, but it can help!

sibit 2y ago

I have always found articles/discussions of code reviews fascinating. In the 10(ish) years I've been employed as a programmer I've never worked in a department with more than ten people and every time the work is distributed such that each person works as an IC on their own siloed project(s). Receiving anything besides the "LGTM" rubber stamp or nitpick comment seems almost impossible.

whynotmaybe 2y ago

> Define (and Stick To) a Style Guide We have some styling rules that are so much in the "Style Over Substance" category that I have a strong tendency to subconsciously categorize them as "not important, at all". Especially when the IDE doesn't respect them when you use the code generation. But as it was a strong irritant for some PR reviewers, and as I couldn't force my mind stop doing it, I automated it by writing a custom linter.

anothername12 2y ago

I have quit places because of their PR process.

mshekow 2y ago

I found two ideas / techniques helpful in this context: 1) Conventional comments (https://conventionalcomments.org/ https://conventionalcomments.org/) as an (agreed-upon) language to be used in PR comments 2) Ship / Show / Ask (https://martinfowler.com/articles/ship-show-ask.html https://martinfowler.com/articles/ship-show-ask.html), where "Show" and "Ship" are non-blocking PRs (or even directly committing to trunk, if you use trunk-based development), since not every(!) PR needs reviewing and/or should block the PR creator

ilaksh 2y ago

The only thing I disagree with him on this is that function length doesn't matter. Longer functions do tend to be harder to understand. I will admit that sometimes a lot of helper functions can make it harder to piece together, so you can overdo it, but usually putting one or two of them back into the main function gives a very readable function. So I would tend towards more smaller more decomposed functions with descriptive (but concise as possible) names by default. This is very different from old-fashioned C standards, which I feel are very outdated for modern tooling and hardware. It's definitely important to have formatting standards to avoid arguments about that. But you are never going to completely eliminate stylistic things because part of that is function decomposition or intersects with the actual substantive design. Because some people feel quite differently about function size for example.

Aeolun 2y ago

Where do people find these places? I have a hard time getting management to accept that more restrictions on who can review is not a bad thing. Nearly 90% of our PR’s have zero comments. I do have a hard ban on formatting comments though. If it’s not covered by the linter/formatter everything goes.

chromanoid 2y ago

Non blocking code reviews ftw https://itnext.io/optimizing-the-software-development-process-for-continuous-integration-and-flow-of-work-56cf614b3f59 https://itnext.io/optimizing-the-software-development-proces... Optimizing the Software development process for continuous integration and flow of work https://medium.com/itnext/how-feature-branches-and-pull-requests-work-against-best-practice-a13a85a016ef https://medium.com/itnext/how-feature-branches-and-pull-requ... How feature branches and pull requests work against best practice

swisniewski 2y ago

I usually adopt a policy with my teams: You are not allowed to complain about style in code reviews. If it’s important enough for your to comment about, it’s important enough for you to add a linter to the build and block the build if the style isn’t met. If it’s not worth enough of your time to add a linter, it’s certainly not worth your peer’s time to deal with comments about style.

scotty79 2y ago

Code reviews are weird. I joined a very small team as a new developer. I got a task, solved it but at code review the senior developer told me to solve it in another specific way. I coded it and it was fine. The situation repeated for few next tasks. It was a bit frustrating because I felt like doing the job twice while "pre-review" of my tasks by the ultimate code reviewer before I started doing them could save both my time and his. But it just wasn't part of the process. Does such thing have a name? A quick check up with a reviewer about whether my plan for implementing the task is align with their preferences?

deleted 2y ago

[deleted]

Andy101 2y ago

[dead]