Talk:Code review/Patch board

Who are the intended reviewers?

People with +2 only? Everyone? Goats?

I'd like to contribute, but don't posses +2 in significant repositories (and lack a drive to acquire them anytime soon). (Meaninglessly adding +1 when +2 are required should probably be avoided if nothing of value is to be gained, it'd just add noise) —Mainframe98 _talk 21:39, 1 February 2023 (UTC)

Its primarily intended for people who have +2 in the repo the patch is in. Of course you're welcome to give helpful feedback to patches regardless of if you have +2. The primary intention is to get stuck things "unstuck" by either merging or giving the creator concrete steps they can do to get it merged. If you can do that without having +2, by all means, but in most cases I think you would need +2. Bawolff (talk) 22:09, 1 February 2023 (UTC)

Awesome idea

I don't have any patches that I'm ready to add here yet, but I must say this is an awesome idea. * Pppery * _{it has begun} 22:37, 1 February 2023 (UTC)

What about using hashtags?

Just an idea. Gerrit has hash tag functionality. Something to consider is if it might be easier (for reviewers and people needing reviews) to tag Gerrit patches with a hash tag and to link that here ? We could also auto-tag patches listed here.

We did this during the wishathon event and I thought it was pretty effective: https://gerrit.wikimedia.org/r/q/hashtag:wishathon Jdlrobson (talk) 20:35, 2 February 2023 (UTC)

Boldly picked "patchboard" TheresNoTime (talk) 00:37, 9 February 2023 (UTC)

Bookmarked! Jdlrobson (talk) 17:03, 10 February 2023 (UTC)

One thing I do like about the wiki-page, is it draws new people in (e.g. via Special:Recentchanges), where gerrit tags are less noticable if you aren't already following them. Anyways, I hope we do both. Bawolff (talk) 18:02, 11 February 2023 (UTC)

Hashtags could be useful, yes. The patchboard name works for me, although maybe we could use patchboard-inbox, patchboard-inreview, patchboard-$random_status to sort the requests according to the review status. A bot adding and sorting the patches on-wiki based on these gerrit hashtags would be great too. In any case, thanks for this. —MarcoAurelio (talk) 15:23, 16 February 2023 (UTC)

Is this thing still alive?

No edits in almost a month, none of the patches here have been reviewed in weeks. Does anyone intend to monitor this page? * Pppery * _{it has begun} 02:42, 28 August 2023 (UTC)

yeah, doesn't seem like it took off quite as much as i would have hoped. Bawolff (talk) 12:08, 28 August 2023 (UTC)

😢 — Frostly (talk) 01:45, 4 October 2023 (UTC)

Retrospective

Since this died, maybe it would be good to have an informal retro to see what lessons can be learned. Maybe at some point we can figure out a better way of achieving these goals based on the lessons learned here. Please everyone fill out your thoughts If anyone has anything they want to say but don't feel comfortable posting it publicly under their name, feel free to email me and I will add it on your behalf.

In addition to adding points to each section, I encourage everyone to +1 any point they agree with and comment on points they disagree with. In the spirit of Five_whys, when reading the points posted, try and ask yourself "why" each statement happened, and if you think you know, add that as well. Bawolff (talk) 13:03, 4 October 2023 (UTC)

What were we trying to achieve?

• Have code contributors who feel stuck waiting for code review have a method for getting unstuck
• Get patches reviewed
• Make the review process more egalitarian instead of relying on being friends with someone with +2.
• [add more]

What went well?

• 24 patches got dealt with: https://www.mediawiki.org/wiki/Code_review/Patch_board/archive Bawolff (talk) 13:03, 4 October 2023 (UTC)
• I think in the beginning it brought together code contributors and reviewers for some good discussion about CR practices. Bawolff (talk) 13:03, 4 October 2023 (UTC)
• After many years of lamenting the lack of code review but not doing anything about it, it was nice to see a bottom-up initiative to actually fix the problem, and that it could gather momentum. --Tgr (WMF) (talk) 04:10, 10 October 2023 (UTC)
• [Add what you think went well]

What went bad

• Interest faded among reviewers as time went on Bawolff (talk) 13:03, 4 October 2023 (UTC)
  • Did people just forget about it? Bawolff (talk) 13:03, 4 October 2023 (UTC)
    • I checked it a few times, but ended up not doing anything due to the issues below. Lucas Werkmeister (WMDE) (talk) 14:07, 4 October 2023 (UTC)
      • I think the bigger problem was patch authors not knowing about it, about it not reviewers. 6 of the 24 patches were by me, and, while that is in part due to the fact that I tended to work on obscure WMF-deployed extensions, surely I am not responsible for 1/4 of all patches needing review. Of course, it's also possible that people were discouraged by it seeming abandoned due to the other issues below. * Pppery * _{it has begun} 19:42, 4 October 2023 (UTC)
• Some patches stayed on the board a long time. Bawolff (talk) 13:03, 4 October 2023 (UTC)
  • I think this contributed to a feeling of lack of forward momentum, which was a bit of a positive feedback cycle. Bawolff (talk) 13:03, 4 October 2023 (UTC)
  • Patches that were large tended to stay on the board a long time. Bawolff (talk) 13:03, 4 October 2023 (UTC)
    • +1 Lucas Werkmeister (WMDE) (talk) 14:07, 4 October 2023 (UTC)
  • Patches where what needed to be reviewed was if we should do something not whether the patch did that thing correctly stayed on the board a long time. Bawolff (talk) 13:03, 4 October 2023 (UTC)
    • +1 Lucas Werkmeister (WMDE) (talk) 14:07, 4 October 2023 (UTC)
  • Patches that already had some reviews made me feel unsure what to do – whether the review was still in progress, or the old reviews were supposed to be ignored, or something else… Lucas Werkmeister (WMDE) (talk) 14:07, 4 October 2023 (UTC)
• There weren't enough patches. Some patches are just not as interesting or too hard or none of the available reviewers feel competent about them, so I don't think "all patches get reviewed" was a reasonable success metric. 4 out of 28 patches did not get reviewed, no one proposed more, and we somehow decided that meant the process failed. --Tgr (WMF) (talk) 04:10, 10 October 2023 (UTC)

  I decided that the process had failed because the page hadn't been edited in 6 weeks, and there was no constructive response to #Is this thing still alive? above. * Pppery * _{it has begun} 05:12, 10 October 2023 (UTC)

  Yeah, sorry, that's fair. I guess what I really wanted to say is, we shouldn't be too quick to assume the problem is that patches didn't get reviewed. Tgr (WMF) (talk) 01:25, 11 October 2023 (UTC)

• I liked this experiment very much and tried to participate both with patches as well as reviews. However, I think there are a few reasons it couldn't work long-term. Being a wiki page it's technically not integrated in what devs typically use. Requests especially don't show up on Phabricator or Gerrit boards. It's also not integrated in existing product management processes but instead relies on the kindness of individuals. Unfortunately this is rarely how decisions on the Wikimedia cluster are made. There is typically a managed team that follows e.g. a yearly goal and just can't spend resources on something completely different. Patches that get stuck are typically not bugfixes – I think these have never been a problem – but suggestions for entirely new features or notable modifications of existing features. These require significant effort: How many wikis are affected? Are the requirements the same on all wikis? Are all communities fine with the change? How much communication and possibly even community consultation is necessary beforehand? Is the test coverage acceptable? What's the rollback plan? And so on, and so on. This isn't about "finding reviewers". --Thiemo Kreuz (WMDE) 19:11, 24 October 2023 (UTC)
• [Add what you think went bad]

Ideas for what might we do differently to prevent the bad

• Bot automation (tags + sorting, per MarcoAurelio)
• Integration into existing workflows (automated alerts)
• Regular visualization of activity: success + status of open patches (can be mostly automated; compare monthly user-talk announcements for ongoing contests) Sj (talk) 17:17, 4 October 2023 (UTC)
• Perhaps in addition to "open" / "merged" a third category can be added for "action needed" with a short description of what action is needed: "existing review needs to be resolved with that review author", "need decision on whether to do this" (aka, needs a "whether" not "how" decision), "too big" (maybe you need to split it up), plus other reasons from the "reasons patches got stuck on the board" list above. Once a patch is moved to the "action needed" category it is "off the board" and you can add a new patch; this keeps the patch board working for the things it is good at while providing at least an itemized resolution for those patches it can't resolve. cscott (talk) 17:29, 4 October 2023 (UTC)
• No reviews for X days means the patch goes off the board (can be re-proposed later). That means the board doesn't get filled up with stale patches, and it motivates authors to cycle their patches. --Tgr (WMF) (talk) 04:10, 10 October 2023 (UTC)

  Agreed. * Pppery * _{it has begun} 05:13, 10 October 2023 (UTC)

• Set a norm about reviewers becoming inactive - if there is no follow-up within X days, another reviewer should feel free to take over. (They should probably feel free to take over anyway, but many people don't like to so maybe an explicit rule makes them feel more comfortable.) --Tgr (WMF) (talk) 04:10, 10 October 2023 (UTC)
• Periodic reminders of open patches where potential reviewers see them (wikitech-l, #code-review on Slack). --Tgr (WMF) (talk) 04:10, 10 October 2023 (UTC)
• Some sort of in-kind reward for reviews? E.g. if you review a patch, you can list a patch of your own, beyond the normal 1/person limit? A bit unfair to people without +2, but trading reviews could be a nice motivation. --Tgr (WMF) (talk) 04:10, 10 October 2023 (UTC)
• [add thoughts]