GNU bug report logs - #66436
[PATCH] doc: Add some guidelines for reviewing.

Previous Next

Full log

View this message in rfc822 format

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 66436 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Maxim Cournoyer <maxim.cournoyer <at> gmail.com>, Ludovic Courtès <ludo <at> gnu.org>
Subject: [bug#66436] [PATCH v2] doc: Add some guidelines for reviewing.
Date: Tue, 10 Oct 2023 20:24:05 -0400

* doc/contributing.texi (Contributing) [Reviewing the Work of Others]: New
section.
* doc/contributing.texi (Debbugs Usertags): Expound with Emacs Debbugs
information and document the 'reviewed-looks-good' usertag.

Series-version: 2
Series-cc: ludo <at> gnu.org
Series-to: 66436 <at> debbugs.gnu.org
Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
Co-authored-by: Ludovic Courtès <ludo <at> gnu.org>
---
v2: integrate guidelines suggested by Ludovic

 doc/contributing.texi | 93 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 88 insertions(+), 5 deletions(-)

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 864190b119..b8a0839c7f 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -29,6 +29,7 @@ Contributing
 * Submitting Patches::          Share your work.
 * Tracking Bugs and Changes::   Keeping it all organized.
 * Commit Access::               Pushing to the official repository.
+* Reviewing the Work of Others::  Some guidelines for sharing reviews.
 * Updating the Guix Package::   Updating the Guix package definition.
 * Writing Documentation::       Improving documentation in GNU Guix.
 * Translating Guix::            Make Guix speak your native language.
@@ -605,7 +606,7 @@ Packaging Guidelines
 * Version Numbers::             When the name is not enough.
 * Synopses and Descriptions::   Helping users find the right package.
 * Snippets versus Phases::      Whether to use a snippet, or a build phase.
-* Cyclic Module Dependencies::   Going full circle.
+* Cyclic Module Dependencies::  Going full circle.
 * Emacs Packages::              Your Elisp fix.
 * Python Modules::              A touch of British comedy.
 * Perl Modules::                Little pearls.
@@ -1972,7 +1973,12 @@ Debbugs Usertags
 tag any bug with an arbitrary label.  Bugs can be searched by usertag,
 so this is a handy way to organize bugs <at> footnote{The list of usertags is
 public information, and anyone can modify any user's list of usertags,
-so keep that in mind if you choose to use this feature.}.
+so keep that in mind if you choose to use this feature.}.  If you use
+Emacs Debbugs, the entry-point to consult existing usertags is the
+@samp{C-u M-x debbugs-gnu-usertags} procedure.  To set a usertag, press
+@samp{C} while consulting a bug within the *Guix-Patches* buffer opened
+with @samp{C-u M-x debbugs-gnu-bugs} buffer, then select @code{usertag}
+and follow the instructions.
 
 For example, to view all the bug reports (or patches, in the case of
 @code{guix-patches}) tagged with the usertag @code{powerpc64le-linux}
@@ -1985,9 +1991,9 @@ Debbugs Usertags
 to interact with Debbugs.
 
 In Guix, we are experimenting with usertags to keep track of
-architecture-specific issues.  To facilitate collaboration, all our
-usertags are associated with the single user @code{guix}.  The following
-usertags currently exist for that user:
+architecture-specific issues, as well as reviewed ones.  To facilitate
+collaboration, all our usertags are associated with the single user
+@code{guix}.  The following usertags currently exist for that user:
 
 @table @code
 
@@ -2005,6 +2011,9 @@ Debbugs Usertags
 appropriate to assign this usertag to a bug report for a package that
 fails to build reproducibly.
 
+@item reviewed-looks-good
+You have reviewed the series and it looks good to you (LGTM).
+
 @end table
 
 If you're a committer and you want to add a usertag, just start using it
@@ -2237,6 +2246,80 @@ Commit Access
 you're welcome to use your expertise and commit rights to help other
 contributors, too!
 
+@node Reviewing the Work of Others
+@section Reviewing the Work of Others
+
+Perhaps the biggest action you can do to help GNU Guix grow as a project
+is to review the work contributed by others.  You do not need to be a
+committer to do so; applying, reading the source, building, linting and
+running other people's series and sharing your comments about your
+experience will give some confidence to committers, and should result in
+the proposed change being merged faster.
+
+@cindex reviewing, guidelines
+Review comments should be unambiguous; be as clear and explicit as you
+can about what you think should be changed, ensuring the author can take
+action on it.  Please try to keep the following guidelines in mind
+during review:
+
+@enumerate
+@item
+@emph{Be clear and explicit about changes you are suggesting}, ensuring
+the author can take action on it.  In particular, it is a good idea to
+explicitly ask for new revisions when you want it.
+
+@item
+@emph{Remain focused: do not change the scope of the work being
+reviewed.}  For example, if the contribution touches code that follows a
+pattern deemed unwieldy, it would be unfair to ask the submitter to fix
+all occurrences of that pattern in the code; to put it simply, if a
+problem unrelated to the patch at hand was already there, do not ask the
+submitter to fix it.
+
+@item
+@emph{Ensure progress.}  As they respond to review, submitters may
+submit new revisions of their changes; avoid requesting changes that you
+did not request in the previous round of comments.  Overall, the
+submitter should get a clear sense of progress; the number of items open
+for discussion should clearly decrease over time.
+
+@item
+@emph{Review is a discussion.}  The submitter's and reviewer's views on
+how to achieve a particular change may not always be aligned.  As a
+reviewer, try hard to explain the rationale for suggestions you make,
+and to understand and take into account the submitter's motivation for
+doing things in a certain way.
+
+@item
+@emph{Aim for finalization.}  Reviewing code is time-consuming.  Your
+goal as a reviewer is to put the process on a clear path towards
+integration, possibly with agreed-upon changes, or rejection, with a
+clear and mutually-understood reasoning.  Avoid leaving the review
+process in a lingering state with no clear way out.
+@end enumerate
+
+@cindex LGTM, Looks Good To Me
+When you deem the proposed change adequate and ready for inclusion
+within Guix, the following well understood/codified @acronym{LGTM, Looks
+Good To Me} phrases should be used to sign off as a reviewer, meaning
+you have reviewed the change and that it looks good to you:
+
+@itemize
+@item
+If the @emph{whole} series (containing multiple commits) looks good to
+you, reply with a @samp{This series LGTM!} to the cover page if it has
+one, or to the last patch of the series otherwise.
+
+@item
+If you instead want to mark a @emph{single commit} as reviewed (but not
+the whole series), simply reply with @samp{LGTM!} to that commit
+message.
+@end itemize
+
+If you are not a committer, you can help others find a @emph{series} you
+have reviewed more easily by adding a @code{reviewed-looks-good} usertag
+for the @code{guix} user (@pxref{Debbugs Usertags}).
+
 @node Updating the Guix Package
 @section Updating the Guix Package
 

base-commit: 5a8f9d32f5196263bc50c2059bac4c4226784a59
-- 
2.41.0

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.