{"thread":{"id":"57402","subject":"Review process improvements: drafting a ReviewingPatches doc","startedAt":"2022-02-10T23:34:05Z","lastAt":"2022-02-11T19:45:10Z","messageCount":3,"participants":["calvinwan@google.com","Junio C Hamano","Calvin Wan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"448202","messageId":"20220210233346.1009735-1-calvinwan@google.com","threadId":"57402","inReplyTo":null,"subject":"Review process improvements: drafting a ReviewingPatches doc","fromName":"","fromEmail":"calvinwan@google.com","sentAt":"2022-02-10T23:33:45Z","receivedAt":"2022-02-10T23:34:05Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Hi all,\n\nIn continuation of Emily’s \"Review process improvements\"[1], I am\ndrafting a ReviewingPatches doc to help standardize the review process\nhere on the git mailing list and to provide a rubric/checklist for new\nreviewers. In order to do so, I am looking to compile examples and\ninput from everyone by asking the list a set of questions biweekly.\nPlease contextualize your answer in terms of whether this was a\nmaintainer, individual, or drive-by review.\n\nMaintainer: in the context of the overall project, is this series a\ngood idea? Does it interact well with other features? Is it\nmaintainable in the long term?\n\nIndividual: in the context of the individual's own experience, is this\nseries a good idea? Is the implementation readable, correct, and\nefficient?\n\nDrive-by / nitpicks: does not answer whether the series is a good\nidea. Makes suggestions for missing pieces, bugs, and/or code style.\n\nHere are the first set of questions I had in mind:\n\nWhat is the best review(s) that you have ever received? What was\nespecially constructive about this review and do you feel like it\ncould be improved upon or is missing something?  Conversely, what is\nthe best review(s) that you have ever given? Feel free to leave\nexamples for each of the review types.\n\n[1] https://lore.kernel.org/git/YbvBvch8JcHED+A9@google.com/\n\n"},{"id":"448231","messageId":"xmqqo83dcnxq.fsf@gitster.g","threadId":"57402","inReplyTo":"20220210233346.1009735-1-calvinwan@google.com","subject":"Re: Review process improvements: drafting a ReviewingPatches doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-11T17:28:49Z","receivedAt":"2022-02-11T17:28:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"calvinwan@google.com writes:\n\n> In continuation of Emily’s \"Review process improvements\"[1], I am\n> drafting a ReviewingPatches doc to help standardize the review process\n> here on the git mailing list and to provide a rubric/checklist for new\n> reviewers. In order to do so, I am looking to compile examples and\n> input from everyone by asking the list a set of questions biweekly.\n> Please contextualize your answer in terms of whether this was a\n> maintainer, individual, or drive-by review.\n\nOften people other than the patch contributors themselves find\nothers' reviews a useful learning opportunity.  I take it that your\n\"contextualize\" request is asking for comments like \"As the\nmaintainer, I found this review by an area expert very helpful\nbecause ...\", as well as \"I sent this patch and a drive-by typofix\nat this URL was not very helpful\"?\n\nWhen soliciting input from the list, it would probably make it\neasier for others if you led by example by sending your answers,\nto show the level of detail you'd find useful.\n"},{"id":"448241","messageId":"CAFySSZDk3s_yD-WOajEucXFiirOJVB2Sa=KMCYHOMRwpkK5VmQ@mail.gmail.com","threadId":"57402","inReplyTo":"xmqqo83dcnxq.fsf@gitster.g","subject":"Re: Review process improvements: drafting a ReviewingPatches doc","fromName":"Calvin Wan","fromEmail":"calvinwan@google.com","sentAt":"2022-02-11T19:44:56Z","receivedAt":"2022-02-11T19:45:10Z","isPatch":false,"sender":{"key":"calvinwan@google.com","avatar":"https://avatars.githubusercontent.com/u/92547554?v=4"},"body":"Thank you for the suggestion Junio!\n\nI will start us off by showcasing an individual review by Jonathan Tan\nI received recently[1]. Overall, I appreciate how thorough Jonathan\nwas with his review. It covers everything from overall architecture of\nthe code to small stylistic nuances in the commit message. The review\ncan be generally separated into two sections: one that goes line by\nline through the entire changeset and suggests possible improvements,\nand then another section that summarizes the overall direction the\npatch author should take to move the patch into a mergeable state.\n\nFor an individual review, I am glad the following was included in the review:\n1. Stylistic improvement for the commit message\n2. Suggestions for clarifying the patch context\n3. Rule for referencing merged code\n4. Variable name correctness\n5. Showcases where certain functions/variables/classes should belong\nfor better structure\n6. Suggestions for code that can be refactored\n7. Makes considerations about edge cases\n8. Determines whether the test cases are sufficient and if not,\nsuggests new test cases\n9. Stylistic improvements of the test cases\n10. Suggestions for overall architecture of the patch\n\nSome items I think that were missing:\n1. The review does not answer whether the overall idea of this patch\nis worth merging. (There is some loss of context here since internally\nhere at Google this feature has been deemed worthy and the legwork for\nthe server side of this patch has already been merged up).\n2. Jonathan jumps straight into the review. A brief summary at the\nbeginning to bring context and to tell the patch author what he/she\ndid well would be nice.\n\nI do many things wrong in my patch, but at no point is Jonathan\ndemeaning towards me. He assumes the best intentions, and makes his\nbest effort to help me improve my patch. To be able to clearly see the\npath forward and to see my patch welcomed on the list motivates me to\ncontinue working on this patch.\n\n[1] https://lore.kernel.org/git/CAFySSZCZfekrdBH1NMArgPLdF4o1KQVY251BLyFgRxp=pDgEOA@mail.gmail.com/T/#m5a033fd28089d41befc937e8adbb133874eee528\n\n\n\n\n\nOn Fri, Feb 11, 2022 at 9:28 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> calvinwan@google.com writes:\n>\n> > In continuation of Emily’s \"Review process improvements\"[1], I am\n> > drafting a ReviewingPatches doc to help standardize the review process\n> > here on the git mailing list and to provide a rubric/checklist for new\n> > reviewers. In order to do so, I am looking to compile examples and\n> > input from everyone by asking the list a set of questions biweekly.\n> > Please contextualize your answer in terms of whether this was a\n> > maintainer, individual, or drive-by review.\n>\n> Often people other than the patch contributors themselves find\n> others' reviews a useful learning opportunity.  I take it that your\n> \"contextualize\" request is asking for comments like \"As the\n> maintainer, I found this review by an area expert very helpful\n> because ...\", as well as \"I sent this patch and a drive-by typofix\n> at this URL was not very helpful\"?\n>\n> When soliciting input from the list, it would probably make it\n> easier for others if you led by example by sending your answers,\n> to show the level of detail you'd find useful.\n"}]}