Document a structured, standardized approach for reviewing code contributions
Closed, ResolvedPublic

Description

This task item was mentioned as one topic in the Developer Summit session T114419 and hence is a subtask of T101686. See also T207.

  • From https://www.mediawiki.org/wiki/User:AKlapper_%28WMF%29/Code_Review#Unstructured_review_approach :
    • Unstructured review approach potentially demotivates first patch contributors, but fast and structured feedback is crucial for keeping them engaged
    • Discuss and decide whether to set up and document a multi-phase, structured patch review process for reviewers: Three steps proposed by Sarah Sharp for maintainers / reviewers, quoting:
      • "Is the idea behind the contribution sound? / Do we want this? Yes, no. If the contribution isn’t useful or it’s a bad idea, it isn’t worth reviewing further. Or “Thanks for this contribution! I like the concept of this patch, but I don’t have time to thoroughly review it right now. Ping me if I haven’t reviewed it in a week.” The absolute worst thing you can do during phase one is be completely silent.
      • Is the contribution architected correctly? Squash the nit-picky, perfectionist part of yourself that wants to comment on every single grammar mistake or code style issue. Instead, only include a sentence or two with a pointer to coding style documentation, or any tools they will need to run their contribution through.
      • Is the contribution polished? Get to comment on the meta (non-code) parts of the contribution. Correct any spelling or grammar mistakes, suggest clearer wording for comments, and ask for any updated documentation for the code"
    • Help (TODO: who? Team-Practices? Release-Engineering-Team? T129842?) WMF teams to implement this process
  • From https://www.mediawiki.org/wiki/User:AKlapper_%28WMF%29/Code_Review#Unhelpful_reviewer_comments :
    • Due to unhelpful reviewer comments, contributors spend time on creating many revisions/iterations before successful merge. Make sure documentation for reviewers states:
      • Reviewers' CR comments considered useful by contributors: identifying functional issues; identifying corner cases potentially not covered; suggestions for APIs/designs/code conventions to follow.[5]
      • Reviewers' CR comments considered somewhat useful by contributors: coding guidelines; identifying alternative implementations or refactoring[5]
      • Reviewers' CR comments considered not useful by contributors: Authors consider reviewers praising on code segments, reviewers asking questions to understand the implementation, and reviewers pointing out future issues not related to the specific code (should be filed as tasks) as not useful.[5]
      • Reviewers' CR comments considered somewhat useful by contributors: coding guidelines; identifying alternative implementations or refactoring[5]
      • (Previous bullet points taken from Amiangshu Bosu, Michaela Greiler, and Christian Bird. 2015. Characteristics of useful code reviews: an empirical study at Microsoft. MSR '15, pages 146-156.)
      • While still using Gerrit and not yet Differential, potentially agree and document how to use -1.
  • Potentially clean up other existing parts of https://www.mediawiki.org/wiki/Gerrit/Code_review

Related Objects

Aklapper edited the task description. (Show Details)Mar 7 2016, 11:52 AM
Aklapper edited the task description. (Show Details)
Aklapper edited the task description. (Show Details)Mar 7 2016, 7:14 PM
Aklapper triaged this task as "Low" priority.Mar 14 2016, 8:51 PM
Aklapper claimed this task.

I'm tentatively assigning this task to myself for the next quarter (April-June 2016) but if someone wants to work on this task earlier, please go ahead!

jayvdb added a subscriber: jayvdb.Mar 15 2016, 8:02 PM
Aklapper raised the priority of this task from "Low" to "Normal".Apr 25 2016, 3:18 PM

Note to myself: Make sure that https://www.mediawiki.org/wiki/Gerrit/Tutorial#How_code_is_reviewed_in_Gerrit only covers technical, Gerrit specific, review aspects and is different from "generic" https://www.mediawiki.org/wiki/Gerrit/Code_review

Aklapper edited the task description. (Show Details)

Status update: I failed to complete this personal quarterly goal in Apr-Jun 2016 - we only made progress in related areas.

When preparing to work on T129068: Improve code contribution guidelines for patch authors (my other personal quarterly goal) I realized that I hadn't taken into account yet another obstacle to solve first which took quite some additional time: T134256: Make Wikimedia's Git/Gerrit documentation less horrible and less scattered.

Hence I've added this task to T131915: Technical Collaboration quarterly goals for July - September 2016.

Aklapper raised the priority of this task from "Normal" to "High".Jun 26 2016, 5:05 PM
Anomie added a subscriber: Anomie.Jun 27 2016, 2:56 PM

Reviewers' CR comments considered not useful by contributors: [...] reviewers asking questions to understand the implementation

On the other hand, if the reviewer doesn't understand it then they probably won't (and shouldn't) merge it. Reviewer confusion can also indicate an opportunity for improved inline comments or a restructuring of the contribution to make it easier to follow.

ksmith added a subscriber: ksmith.Jun 27 2016, 4:40 PM
greg added a subscriber: greg.Aug 4 2016, 5:44 PM
greg edited the task description. (Show Details)Aug 4 2016, 5:50 PM

18 edits later, https://www.mediawiki.org/wiki/Gerrit/Code_review looks a bit better.

Still to do regarding that wiki page:

Performed some more edits on the docs for reviewers. Regarding my personal involvement, I consider the documentation part finished.

I would like to reassign this task to Engineering to lead efforts to have teams follow those guidelines, establish a routine of going through unreviewed patches, and other iterative improvements.

Qgil added a subscriber: Qgil.EditedSep 15 2016, 7:43 AM

Performed some more edits on the docs for reviewers. Regarding my personal involvement, I consider the documentation part finished.

Very good! This was a long way, thank you.

I would like to reassign this task to Engineering to lead efforts to have teams follow those guidelines, establish a routine of going through unreviewed patches, and other iterative improvements.

Since the theoretical agreement and the documentation have been completed, I think it would be better to resolve this task and then discuss with @greg / ReleaseEngineering / Technology & Product what are the next steps that they should lead because our time as leaders here has come to an end.

We have two open tasks that could host this discussion:

For what it's worth, I think T78768: Goal: Agree on and implement actions to prioritize code review of patches submitted by volunteers and T113707: Decision on Wikimedia Foundation service-level agreement on code review times are too specific to host the more general discussion.

I would prefer to see a phab task that spans code review of both staff and volunteers (CR is an ongoing pain point even within teams), and focuses on the overall quality of code reviews, rather than just timeliness. The two issues above could be subtasks or just related tasks.

Aklapper changed the title from "Agree on + document a structured, standardized approach for reviewing code contributions" to "Document a structured, standardized approach for reviewing code contributions".Sep 19 2016, 5:22 PM
Aklapper closed this task as "Resolved".

I wonder if we could just broaden the scope of T78768?
T113707 feels just like one potential outcome.

Qgil awarded a token.Sep 20 2016, 12:07 PM
Qgil added a comment.Sep 20 2016, 12:09 PM

I have no strong opinion about how to organize the next steps in Phabricator. I do think it is useful to keep T78768 with a specific focus on volunteer developers. As much room for improvement there is in code review between professional developers, they still have many ways to communicate and discuss with each other that most (new) volunteer developers don't have access to or clue about.