[RFC] Improve code-review process for clang-tidy

Clang Frontend clang-tidy
1

Hi,

I’m writing with the hope of starting a discussion on how to improve the code-review process for clang-tidy patches. My experience so far as a developer has been rather frustrating lately. Here’s a summary of how it goes:

  • I put up a patch for review.
  • Very quickly (within hours) I get reviews from multiple people.
  • I address the comments.
  • Then reviewers go into complete silence for weeks or even months.

At this point, I’m honestly confused as to what’s happening:

  • Do reviewers get any notification when the patch is updated?
  • Do reviewers get any notification when I ping and tag them?
  • Do reviewers really don’t have time to at least communicate a rough ETA for review, or tag other reviewers? I understand people are busy, but I think it’s not unreasonable to expect a response (not review) within 1 week when people pings and asks questions.

Due to this, as a developer, I feel discouraged to contribute to clang-tidy. I fear others
might experience the same, leading to clang-tidy contributions being reduced over time.

How can we improve?

Looking forward to hearing your thoughts!

5 Likes
2

It seems I’m not alone alone here, see for example here: patch blocked for 4 months due to reviewers not providing feedback:
https://reviews.llvm.org/D128372

@njames93 , since you are the Code Owner in clang-tidy, would you mind sharing your thoughts?

3

I think reason is obvious: other developers are busy with something different. Some are currently working with different parts of LLVM Project (Clang, Clangd, Include-cleaner), some - outside. It may be reasonable if LLVM organization will sponsor/employ some maintainers (like Linux Foundation does), so they could have more time to work on its projects.

1 Like
4

Still, the code owner is supposed to take responsibility for making sure reviews happen. If there is a code owner, and they aren’t doing the job, that’s a problem.

5

Denis and I been pinging D128372 weekly for months. I’ve also tagged @njames93 in the LLVM Discord, but no reviews have been made.

It’s incredibly discouraging to developers for projects to have code owners that are non-responsive for months on end. If their attention is taken up by other projects (which isn’t actually obvious given that @njames93 seems to have provided commentary to other clang-tidy contributions in recent times), then it’d be great to signal that instead of having contributors remain in limbo indefinitely.

6

But other quite active developers were also added to this particular merge request as reviewers, so it’s hard to claim that Nathan is bottleneck.

By the word, I always try to add developers with proven record of contributions to new Clang-tide patches on Phabricators. But this is not silver bullet in all cases.

1 Like
7

For me it’s extremely confusing that I can get quick reviews within hours from many reviewers (which I’m infinitely grateful for!), and when everything is fixed and ready to merge, suddenly there’s not a single reviewer that can give any more feedback or approve the patch. Patches remain in limbo, merge conflicts happen, and in general they rot.

If the issue was that the Code Owner has no time, then he wouldn’t review any other patches, right? Therefore I’m more inclined to think that notifications are the problem here (either they don’t arrive, or they are ignored).

Sure, there are other reviewers, but are they all waiting for the Code Owner to give the final approval? There’s no guidelines that require so, but perhaps it’s not clear either that patches can be approved without the approval from the Code Owner.

It’s very sad to see patches either remain in limbo, or be self-approved by the patch owner, due to this.

8

@AaronBallman do you have any thoughts on the matter? The current Code Owner for clang-tidy was nominated by you IIRC.

1 Like
9

First off – thank you for raising these concerns, and I’m sorry that you’re having a frustrating time getting your patches reviewed for clang-tidy changes. I’m partially to blame for this bottleneck; I used to do a ton of code reviews in clang-tidy, but I had to pull back from doing those reviews due to time constraints and a need to focus elsewhere. I nominated the other folks in clang-tidy who had been doing a significant number of high quality reviews and was happy that one of the folks (Nathan) volunteered to step up.

To answer some of your questions:

  • Do reviewers get any notification when the patch is updated?

Yes, by default Phabricator sends emails to everyone listed as a reviewer or subscriber.

  • Do reviewers get any notification when I ping and tag them?

No more notifications than they would already get by being a reviewer/subscriber (basically, tagging someone automatically subscribes them to the review, so they get the usual email from that).

  • Do reviewers really don’t have time to at least communicate a rough ETA for review, or tag other reviewers? I understand people are busy, but I think it’s not unreasonable to expect a response (not review) within 1 week when people pings and asks questions.

It depends on the reviewer, really. Some of the code owners are folks who are sponsored to do this work by their employer, and some of the code owners are folks who are purely doing this on a volunteer basis. This means some code owners are going to have more time to dedicate to reviews than others, which leads to inconsistent review throughput. But it also keeps the community healthy by not having corporate sponsorship be a requirement to be a code owner.

Plainly: we need more volunteers to do code review and we need to say “no” more often. This isn’t unique to clang-tidy, FWIW, we run into the same sorts of problems in Clang and other parts of the project.

There are far more people submitting patches then there are people reviewing patches, so code reviewers (and especially code owners) generally have a significant backlog. If the code reviewer is also a volunteer, then the backlog problem gets worse. Add in things like illnesses and vacations, other work responsibilities, etc, and you probably can better see why it’s a challenge to get everything reviewed in a timely manner. I think clang-tidy suffers from this problem more than other parts of the project because it’s very easy to write new clang-tidy checks but each check takes a fair amount of time to properly review.

What’s more: it takes time to build up the knowledge required to feel confident to approve a review, so getting new code reviewers helps a bit, but doesn’t remove the bottleneck immediately.

So, to me, there are some short-term solutions and some long-term solutions. In the short term, I would recommend:

  1. Have patience with code owners, especially because it’s not always obvious who is doing it purely in their spare time vs who is being paid to do it. Review throughput is an issue we’re aware of as a community and have been actively working to address (we added and updated a number of new code owners this year), but we know we have more work to do and that will take some time.
  2. Communicate better on reviews. If you don’t need something reviewed right away, put [WIP] in the patch title or say something in the patch summary about it not needing review yet. If you’re a code reviewer on a thread and you see someone “ping” it after a week+ of no activity on the review, at least respond to that with something acknowledging that you’ve seen the ping to let the author know they’ve not been forgotten. That sort of thing.
  3. Say “no” more often. This is very hard to do as our community does not have a culture of saying “no” to things, but we only have so much bandwidth and failing to recognize that will only lead us to a lower-quality product in the long run, so IMO, we really need to get better at this. This “no” does not have to come from the code owners! Anyone can question whether a new feature is worth the cost, so if you see feature work you think will take significant time (for example, adding a new module to clang-tidy or large new extension to Clang) and you’re not certain why it’s being added, ask questions and see if maybe that’s a feature we should be waiting to add until later. The “no” doesn’t have to be a permanent no, it can be a “not yet, come back later (perhaps with more details about )”.

In the long term though, what we really need is:

  1. Perform more code reviews yourself. Ultimately, the only solution to our lack of review bandwidth is to add more reviewers. Performing code reviews for others is a fantastic way to give back to the community as well as a great way to learn the code base. You don’t have to feel confident enough to give “final approval” or be a code owner to help out here – literally any constructive comments you add to a review are helpful! And if you approve the review but still want someone else to look at it, it’s perfectly fine to say so in the review. Remember, code owners do not have to be the final approver for a patch; it’s fine to land a patch once the reviewers and authors are confident in it, the code owner can always raise concerns post-commit if they have any.
  2. Encourage others to perform code reviews as well. If applicable, talk to your coworkers who are also working on the project, talk to your manager, etc to encourage them to dedicate some time to performing code reviews. If you notice someone new to the community doing reviews and they have questions about anything, help get them answers (even if it’s just connecting them with someone else who might have the answer).
  3. Nominate additional code owners where appropriate. If you see someone giving a lot of high-quality reviews in a particular part of the project, ask them if they’d like to be nominated as a code owner and do so if they seem interested even if there’s already an existing code owner. We’ve been trialing having multiple code owners for components and it’s gone pretty well thus far. Having more code ownership helps spread the work around, gives people more of a sense of recognition for their efforts, reduces the risk of critical knowledge being focused in just one or two people, etc

Speaking for myself, that’s not always the case. Some reviews are easier than others and there are times when the only thing I have time/head space for are trivial reviews. So you might get early mechanical feedback from me (typos, coding style issues, test requests, etc) very quickly and then it may be a week or more before you get any actual review feedback (technical concerns, correctness issues, etc). I do this because I try to get people feedback as quickly as I can and I figure some minor feedback is better than silence.

I agree, but speaking as someone who does a ton of reviews, there’s a serious need to reduce the amount of friction involved in signaling that. For example, I’m taking off the rest of this month starting on Fri. Do I respond to all ~50 of my reviews by removing myself as a reviewer since I can’t get to them in a timely manner? Do I leave a comment in every single review (instead of, you know, reviewing the code…)? At some point, there’s really no good way to say “I’m busy, it’ll be a while before I get to this”. I have a dream where there is some sort of status indicator next to each code reviewer’s handle on a review with a green/orange/red/black indicator for generally available/potentially busy/definitely busy/out of office that is automatically set based on your review backlog or manually set (for things like vacations). But alas, I don’t know of a code review tool that has anything like this.

9 Likes
10

Thank you so much for the lengthy answer, I really appreciate you taking the time! It brings me a lot of perspective on the problem and I agree with your short/long-term solutions!

It depends on the reviewer, really

To clarify, I didn’t mean “do reviewers don’t have time to review?”. I meant do reviewers not have time to communicate anything at all within 1 week or even months? Essentially we are currently unable to contact Nathan by any means. We have pinged him on patches, tagged him in this post (which he hasn’t answered) and as they mentioned above he doesn’t answer on Discord either.

This contrasts with the fact that he is active in reviewing new patches, so he’s not completely missing. Acknowledging that he receives pings takes less time than to do a patch review, right? Should we start communicating with him by putting up new patches for review, since that’s the only thing he answers to? Being able to communicate (not review!) within 1 weeks seems like a fairly basic requirement for a Code Owner IMHO.

we need more volunteers to do code review

Fully agree! But it’s still unclear what the role of the Code Owner is. Must the Code Owner have looked at the patch at least once before merging, or can patches be merged without any involvement from the Code Owner?

Also, I currently tag quite a few “usual reviewers” - they also go silent after the first review! So I wonder: are they waiting for Nathan to give the approval? Why can’t they approve the patch themselves? Even trivial patches remain in limbo for a long time.

If you’re a code reviewer on a thread and you see someone “ping” it after a week+ of no activity on the review, at least respond to that with something acknowledging that you’ve seen the ping to let the author know they’ve not been forgotten.

+100, this is exactly what I’m currently missing from reviewers. I don’t think this requires so much time from reviewers.

But alas, I don’t know of a code review tool that has anything like this.

Couldn’t we just have a post here in Discourse where each Code Owner writes their availability, like it’s done for Clang?

11

I agree that communication can definitely be improved (I mean, that’s true for all of us, really!). But please keep a few things in mind: 1) clang-tidy doesn’t have many people who do a significant amount of reviews for it to begin with; when I looked for code owners, I had one person step up who felt qualified. 2) we get whatever time and effort people are willing to volunteer. 3) patches do not require code owner approval to be landed.

One of the points I was trying to make earlier is that, at least for me, there comes a point when leaving a comment on every review I have open means there are several less actual reviews I can get to in a day because I’m spending all my time responding to pings with “sorry but I can’t do that right now”. (N.B. someone on IRC yesterday mentioned their incoming review backlog is ~300 reviews; you can spend all day managing pings and get no review work done.) What’s more, because reviews come in at a somewhat consistent pace, once you get behind it can be very hard to catch back up. So that “sorry I can’t do that” is something I’d likely have to write pretty much on a daily basis, which I’m certain would chip away my enjoyment of doing code reviews.

That’s not to say we shouldn’t be trying to improve communication, of course. But please continue to have patience with the volunteers who step up to do code reviews. While the pain point you are feeling is the lack of communication from someone, I think the root cause of the pain is actually that we’re putting too much work on one person’s shoulders. Hopefully folks in the clang-tidy community will see the need and have the ability to step up and help out with doing more reviews.

The code owner is there to: 1) make decisions when patch authors and reviewers have intractable technical disagreements (doesn’t come up particularly often), 2) ensure the architecture of the component they’re responsible for is not being compromised, 3) conduct reviews or finds reviewers for the patches in their area of the code base.

A code owner doesn’t have to be involved in a patch at all in order to land the patch. If a patch author feels confident that the folks giving the LG on their patch are competent, go ahead and land the patch. If you’re not confident, ping the code owner owner. But if you land something on someone’s LG and it turns out to not actually be good, that’s not the end of the world – we catch things in post-commit review, the bots catch things, we encourage reverts, etc. Btw, we document this here: LLVM Code-Review Policy and Practices — LLVM 16.0.0git documentation

They can approve the patch themselves, but not every reviewer feels comfortable signing off on patches. e.g., they may make a pass over the patch for mechanical issues (coding style, typos, etc) and that’s all the more comments you’ll get from them, or they may be newer to the community and not feel empowered to approve a review, etc.

While I understand (and largely agree with) this perspective, I also understand the reviewer perspective where 1) it can actually take nontrivial time to respond, and 2) it’s socially awkward to publicly say “sorry I’m not getting this done on your timeline”, but even more so to have to repeatedly say it week after week.

Maybe. We’ve never had a post like that before, so it was an experiment I was trying this year to see if maybe this would work (so far, we’ve had three code owners out of 30+ respond and one non-code owner response). But to be honest, I waffled about posting it because I felt awkward and I wasn’t certain it would scale well. I suspect that self-selecting yourself to be “important enough” to convey your availability in a community-wide post will wind up being biased towards only the most outgoing members of the community.

1 Like
12

My personal take is a slightly different approach. Of course as a reviewer and code owner we always accept that we can be better and address things in a more timely manner but.

As a reviewer I like to “Accept” things from people I trust, and that trust doesn’t come overnight, but its fairly easily earned.

So often the contributors first submission is some massive feature, Its their dream feature… “I wish clang-XXXX could do this!” You’ve probably worked on your huge idea for weeks learning the code base as you go with little to no feedback because you didn’t contact us because you wanted your feature to make a splash…

You submitted your first review with a “Ta daa”… here is the best thing to be added since sliced bread… (I know this behaviour because I’ve done this myself)

Ultimately as code owners we have no idea if you are here just for this one commit (drive by) or if you are here for the duration. Do I want to land your work of art? Do I want to consume that body of work into my responsibility. Have you followed the coding conventions, do I want to read your work of art or do I want to continue with my own submission or fix bugs, given the little time I have to work on this project myself…

At this point I am very reluctant to accept your review. Not because I think your submission isn’t valid or good but because I’m trying to see how committed you are… I want you to correct your review, I want you find your own bugs and resubmit the patch. I want to give you 1 or 2 comments then see if you come back…because SO many reviews we spend time reviewing only for the submitter to disappear because we didn’t get to it immediately.

Honestly its a little bit of a test for my perspective! are you a committed submitter who will:

a) manage your review to completion
b) have patience to see your submission land.(no matter how long it will take)
b) help in the future fix other peoples bugs
b) look after your own feature in the future
c) do my reviews

Ultimately this comes down to expectation as to how quickly you’d like to see you patch landed. Honestly what is the rush? I’ve had personal code reviews live for a very long time before they land (1+ year). For me its more about the perseverance and longevity of the contributor.

The thing with these tools is you don’t need to commit them to see them being used, Its much better if you make your change locally and then use it extensively first so you iron out all the bugs. I understand the rush to commit, I felt it myself in the early days (especially to get that feeling of your first submission), but I have to say take it slow there is no rush. (Its a Marathon not a Sprint)

As a code contributor for clang-format, I know that a code change that gets committed might not actually make it to mainstream usage for 3 or 4 releases. If you look at most projects using clang-format they are rarely on the latest release. (we are getting bugs coming in for 13/14 when we are about to goto 16). The problem is if we rush to commit incorrect code, it could be very difficult to patch these versions so we need to be very sure that the changes are good. We could break an awful lot of open source projects. (It is not uncommon for project that run on the bleeding edge such as Firefox to be the ones that Thankfully find these early for us!)

Ultimately contributors are put off, they get fed up and leave their review. (not even abandon it), its not ideal but the reality is they failed the patience and longevity test, they would have left anyway in that case and then we would be lumbered with their submission and someone else would have to fix their bugs. Do we need their submission? (its not like we don’t have it? as its in the review, if its a good idea it will rise to the surface and someone with take it on).

So how to over come this, is not about the reviewer giving more time, its about the submitter giving more time… you have to be realistic, you have to win the reviewers trust first. You need to become part of the team.

At this point your going to say you don’t have time to do that, We understand that we are in the same boat, and its why if there are only 3 or 4 of us doing the reviews its why we can’t get to yours in a timely fashion. (cause and effect!)

However…

We recently had someone join the clang-format team, and that person did so without their first commit being a huge submission. That person quickly won our trust, and has become a regular contributor and code reviewer.

That person gained our trust by first being in the git issues, by working through the issues, in some cases just triaging them or finding duplicates and adding labels. It got them noticed, they also started to send reviews with very small one liners, bug fixes, They looked at other reviews to see who to add, and who would give them the quickest feedback and I’m pretty certain they also looked at the comments we were giving to others because their submission seemed almost perfect first time, and most importantly they were not trying to land a 1000 line new feature. But they proactively became a member of the community before doing the next biggest thing. This was highly refreshing…

I know for one I do the reviews of those regular reviewers/contributors first. With limited time its them I’m doing, because I know their review will likely be good and its unlikely to be huge.

So from my perspective ask yourself as a submitter what can you do to help the teams become better, what can you do to become a reviewer or trusted advisor…what can you do to free up the experienced code owner to work on what they are doing because it could be amazing (as they know the code better than you!)… If your review isn’t getting noticed… then get yourself noticed first, befriend a reviewer or review others. (not just comments, be brave and accept some)

Help out and you might just be able to drop in a “could you look at my review”

MyDeveloperDay

4 Likes
13

Thank you for your feedback! It’s very interesting to hear the different perspectives. I understand time is limited and is natural to review “code that you already expect to be good from a trusted individual”, and I fully agree that individuals should be expected to continue to maintain their feature over time, instead of just dumping their code (and bugs) to others.

Honestly what is the rush?

I can think of some scenarios:

  • Patches that remain in limbo might run into conflicts that need to be solved over and over, or perhaps they might even need to be completely redone as the repo progresses.

  • Say you introduce a new clang-tidy check and when it’s out in the wild you discover some bugs or that the functionality is not exactly what was intended. Then you really should aim to get that fixed before it’s released, otherwise it’s impossible to fix them due to the need of keeping “backwards compatibility”.

Contributors don’t come to LLVM to “pass a test” - they come to improve the tools they love. If their contributions aren’t valued, they won’t contribute. Ultimately, the project needs contributors, or it will inevitably end up dying. So if the problem is “we lack contributors” I think the solution is to lower the entry bar and provide a safety net for newcomers to make mistakes.

so we need to be very sure that the changes are good

I see this as a case of “perfect being the enemy of the good”. We are humans, we make mistakes. We can put as much effort as we want on making things perfect but there will always be something we miss. There I think there is value in being able to have fast iterations.

14

Contributors don’t come to LLVM to “pass a test” - they come to improve the tools they love. If their contributions aren’t valued, they won’t contribute

For the record its not LLVM’s test its my personal test of people willingness to continue to contribute, Being a code reviewer I’m afraid is all about judging things. I have to judge if the contribution is worth it, part of that I’m afraid is the contributors willingness to be here to help when that contribution goes wrong (because, I agree people are human and hence it will. So we NEED to know the contributor will be here so there is a timely fix)

I suspect the review delay might be greater for “first” or large submission (or controversial ones), unfortunately this tends to be what contributors do for their first time. Its not a good place to start is what I’m saying. Start small, this is what you’d do at a new job, you’d fix one or two small things before moving onto something bigger. I know you want your dream feature… you can have it locally. Maybe just don’t lead with that.

Honestly I don’t have any experience of trusted members of the team having any real problems of reviews taking THAT long, of course they take longer than we’d like, but not months of inactivity,

Saying that its not uncommon for our own work to take weeks to get through the process, I just get the sense that those with experience accept that this is the lead time. I for one park the review and move onto another issue/bug, while others mull it over.

“perfect being the enemy of the good”

I don’t feel this is the case for me. but I don’t accept that we should drop the barrier of entry to below good, just because people are impatient to commit, I start to question their motives.

There I think there is value in being able to have fast iterations

Why do those fast iterations have to involve committing to main and impacting 100s if not 1000s of people maybe adversely, doesn’t the local build environment give you everything you need?

Can’t you run clang-XXXX against large open source projects before you commit to flesh that out, (I know that’s what I did when I made a clang-tidy submission)

I think the reviewers have a responsibility to try and find ways to minimize the number of “drive by patches” that are accepted, in my view these are not what the project needs. In my view those contributions are not as valued as the contributor might believe.

I sort of feel, the problem you describe are the problem of new contributors, I feel that’s made clear by your comment “Contributors don’t come to LLVM”, if you are a regular contributor you are already here and you know the modus operandi and either accept it or you’ve moved on to a more fast paced development project.

If what we are saying is there is a high barrier of entry for new contributors to get their first piece of work landed. I might be tended to say… “Yes you are correct there is, and perhaps there should be”, but I’d also say follow the guidance of doing the “good-first-issue” items, and don’t lead with your “grand design”.

But I feel that guidance is documented elsewhere its just we tend to ignore it because we want to get our amazing idea in. (me included).

My 2c

15

Agreed. However, how do reviewers communicate that to contributors? There’s no such mention in any guidelines. This comes a total surprise for new contributors.

IMHO, the least reviewers could do, instead of leaving patches in limbo, and simply not answering pings, is be clear and say “look, you are not a trusted developer. Make sure you do X and Y first to gain our trust, only then will we have time to look at your code”.

I know you want your dream feature

I disagree with the language. This post is about “clang-tidy”. Most entry-level developers will do one of two things:

  • Fix a bug in an existing check.
  • Add a new check.

Both of which are fairly small patches that cannot be simplified further. One cannot add “half a check” today and the other half tomorrow. There’s even a script “add_new_check.py” that does exactly what needs to be done - no more, no less. So I wouldn’t exactly call most contributions to clang-tidy “grand design”, “work of art”, “dream feature”. clang-tidy is designed to be easily extensible and I perceive it as a welcoming environment for contributors.

but not months of inactivity,

That’s exactly what I’m raising here, months of inactivity during review :slight_smile:

are impatient to commit, I

Not impatient to commit, impatient to get feedback. I don’t see a reason to hold a patch indefinitely if there’s no more feedback to add to it. Newcomers cannot learn without feedback either.

I think the reviewers have a responsibility to try and find ways to minimize the number of “drive by patches” that are accepted

Sure, but they will always make mistakes, no matter what. I think it’s a fact that need to be accepted.

if you are a regular contributor you are already here

I have the feeling that it’s a chicken-and-egg problem. In order to join the “club” you need to have contributed. But in order to contribute you need to be part of the “club”.

“good-first-issue” items

As of today there’s only 9 such issues for clang-tidy. I find it hard to increase the clang-tidy community if this becomes a pre-requisite to contribute.

In general I find it sad that reviews are taken into account based of social aspects (who is part of the “club”, who are you friends with) instead of technical merits (does the patch make sense, does it follow the guidelines, is it well tested, etc).

1 Like
16

Its certainly not meant to be a “club”, I don’t actually know any of these people, I’ve never met any of them in person. (sadly)

But what I would say is like clang-format, there are 671 open issues for clang-tidy, we are drowning in open issues which we can’t get through with our current levels.

My advice to almost every new comer is please please start there. Please do something small.

And @carlosgalvezp from your Phabricator history, you are clearly capable of writing new checkers and hence reviewing code and even in my view accepting a review for clang-tidy.

My advice is if the clang-tidy code review process isn’t fast enough, be the person who makes that change, start leaving comments and start accepting reviews. I feel that’s how I started, it took some time and as a reviewer I missed things (and still do), but my hope was that by doing that it was part of the reason why the original clang-format developers felt able to move on a little.

You may find the current owners feel burnt out by the review burden, carrying some of that burden might help, both the owners and clang-tidy could benefit as well as you.

17

I have to disagree here. We should not be trying to exclude drive-by patches. These are often fixes for something that the contributor tripped over, and have value to the project. Someone who successfully contributes a patch might well feel encouraged to do it again, and possibly over time become a regular contributor. This helps the project grow, which is a positive thing. Of course someone might contribute their one patch and never be seen again; but if it’s a good patch, that’s okay too.

(I’m finding this an interesting discussion because I myself am working on what is turning into a fairly large patch to another project; it is taking a long time (years) because it is a side project and I cannot devote a lot of time to it. The feature will have value to everyone who uses this other project, but if it gets rejected because I am not a regular contributor, that would be very sad.)

2 Likes
18

Ok, let me clarify, I don’t consider a bug fix to be a drive by patch. (honestly if people contribute bug fixes generally I’m delighted!)

19

we are drowning in open issues which we can’t get through with our current levels.

Right, I see how it’s important to fix issues before adding new features, and reviewers will prioritize that.

code review process isn’t fast enough, be the person who makes that change

Absolutely! As I said however my main concern is not “speed of review”, but rather the lack of communication between contributor and reviewers, which leads to patches being in limbo. I.e. it happens that reviewer A leaves comments, I fix them, and then I never hear back from A. Can a reviewer B accept the patch, or should they wait for A to confirm that they are happy with the addressed comments? What if further discussion is needed?

honestly if people contribute bug fixes generally I’m delighted!

Good points! I think it’s important to distinguish between a) bug fixes and b) new big “dream feature” patches. My biggest concern is related to bug-fix patches which I believe there is certain “rush” to get merged before release. New features are of course not so urgent, and links to what Aaron said above regarding “saying ‘no’”.

Would it help reviewers and speed up reviews of bug-fix patches if commit messages were clearer in that regard? Would even a special tag make sense?

20

Is… is this talking about me? If so, I feel like you are being way too nice about me

1 Like