Review time and the bus factor of helix

[poliorcetics]

As a start, I like Helix a lot and this discussion does not intend to dunk on anyone or disparage people. I'm aware that open-source is motivation-based 90% of the time and that I can't expect people to my schedule and wants. This is not a rant issue.

Current situation

Helix currently has a small bus-factor of two, with @archseer and @the-mikedavis being the only one merging PRs. They may not be the only ones with the right to do so, but they are the only ones exercising it.

At the time of writing, there are 95 PRs passing CI open, (though maybe missing a rebase). A third of them have been last updated more than a month ago.

The problem

PRs take a long time to be reviewed and even longer to be merged. I personally have two PRs open, #3347 and #3690 and neither has received any review from a member of the helix-editor team, the first in more than a month. While that doesn't affect me much, I can easily see it discouraging other (potential and real) contributors, especially if this is their first PR to helix, for some trivial fix or improvement already approved in an issue.

I have no data on this but it seems bigger PRs are also the more likely to fall through the cracks and be ignored. This is logical, a bigger PR is more work to review, but the people writing big PRs are also the people most likely to help review/invest time in the project and they should not be made to feel useless.

Working towards a solution

A recent talk by Alice Cecile at RustConf explains this much better than I could but the gist of it is: PRs are currently merged dozens of times quicker if the author is "known" and PRs from new/small tie contributors get stuck in review hell, with no response in sight, even to say "no".

I'm not saying there should be a promised date of review for PRs, nor more people in the helix-editor organisation, but there should be a clearer process to signal when a PR is ready for review and the subsequent review should come much quicker than currently (though not at the expense of the health/time of the reviewers).

This could be done by asking people that have contributed several times if they want to help review. If they approve of a PR, a member of the core team can come take a look, introducing a filter system that would decrease the charge on the core team while making more people aware of the internals of helix and able to help over time.

Having people to label issues & PRs, in effect triaging them, would also make it clearer what is worked on for the future of helix.

---

Anyway, I hope that did not come through as insulting or rude, I genuinely like helix and I would like to see it succeed long term as an editor 😄.

[archseer]

The problem with a lot of PRs is that they either require discussion, or they were done quickly with the contributor solving their specific need and not considering if the code fits within the crates as a whole. Reviews also grew in complexity, because we started with very small (<5 line) fixes for panics, but now it consists of larger features.

As a whole, the pace is still really fast, and we merge more PRs than we leave open. It's just that the amount of PRs is increasing: https://github.com/helix-editor/helix/pulse/monthly

I'm currently either doing review from the oldest PRs forward (skipping any that are pending), or quickly merging newer PRs that I can review in a few minutes.

We started using S-waiting-for-author, S-waiting-for-review and S-needs-discussion/-testing to triage PRs better, but it's still a lot of manual effort to keep up to date.

[poliorcetics]

Reviews also grew in complexity, because we started with very small (<5 line) fixes for panics, but now it consists of larger features.

Yes, that's why the situtation shouldn't be "wait for 2 months because we only have 3 or 4 reviewers", nor "we accept everything without thinking", I'm specifically saying we should try to organize and find a middle ground. Some people on rust-lang/rust do it by asking people that have contributed a few times to review a new PR, letting them know it's okay to review PRs even without merge rights.

This approach would probably help you since nits and little mistakes would be fixed before you review, making it easier to focus on the bigger picture, like integration into helix.

As a whole, the pace is still really fast, and we merge more PRs than we leave open. It's just that the amount of PRs is increasing

That's nice, and another symptom: as helix grows, having more reviewers/triagers can only help :)

We started using S-waiting-for-author, S-waiting-for-review and S-needs-discussion/-testing to triage PRs better, but it's still a lot of manual effort to keep up to date

Yes it is, is there a way to let people that have already made one or two contributions set labels for their PRs/issues without giving them full rights ? This along with a bot message on each explaining what are the likely labels to set would probably help a lot !

[archseer]

I agree, it's just hard to garner enough interest for contributors to jump in and review other PRs. I initially added more maintainers but they became inactive/don't look at the PR queue often. It does occasionally happen though, and it's nice since it does save me time by catching small issues ahead of review time

[poliorcetics]

Either by hand or a bot, you (plural, maintainers) could try to ping people that have signaled interest. I cannot give hours everyday but I already read all commits to see what was added since I'm always on the day's master and ensuring my #3690 stays up to date. I could review smaller things, and I'm sure several other people would feel more included if they were asked for small reviews and such.

It's still a difficult job and not the most interesting one too, especially for developers, managing people is often boring to me so I understand.

[gibbz00]

Apologies for the necrobump, but for anyone reading this in the future:

The problem with a lot of PRs is that they either require discussion, or they were done quickly with the contributor solving their specific need and not considering if the code fits within the crates as a whole.

A tip against this would be to hop into https://matrix.to/#/#helix-community:matrix.org and ask for suggestions before beginning to work on a PR :)

[CBenoit]

I watched the talk you linked and here are some interesting points we could maybe learn from:

• Triage by controversy (S-needs-discussion is pretty similar though)
• Encourage community reviews

Also, writing down policies in some areas could help a lot (for instance, I'm unsure of the policy for including a new theme in the main repository). The recent example I saw was rustc's Target Tier Policy.

[kirawi]

We could introduce a PR template to help explain the objective and solution of each pull request. Part of the issue I'm observing is that these details are ambiguous in many PRs, and can make it harder to review. It can certainly make it more work for the reviewer if they're unfamiliar with the altered code.

It takes a lot of energy to review other people's code, and reducing the effort involved can make it more appealing to do.

[Omnikar]

From what I'm seeing it seems like PRs are prone to becoming inactive after reviews take an indefinite amount of time to arrive and the PR author doesn't look back at the PR. I don't know how practical it would be, but I think at least providing an estimate of review wait time to PR authors, even if it's long, would be very helpful.

[poliorcetics]

I don't like this approach because it pushes calendar constraint on open-source/hobby time, maintainers shouldn't be pressured into such, that's not good at all long term planning IMO.

[Omnikar]

Right, good point.