diff mbox series

[bug#66436] doc: Add some guidelines for reviewing.

Message ID 19f82d9bbef649c750ad067d23ebbaee6f9ae494.1696942467.git.maxim.cournoyer@gmail.com
State New
Headers show
Series [bug#66436] doc: Add some guidelines for reviewing. | expand

Commit Message

Maxim Cournoyer Oct. 10, 2023, 12:54 p.m. UTC
* 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.

Change-Id: I56630b15ec4fbc5c67e5420dbf2838556a005d6b
---
 doc/contributing.texi | 53 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 5 deletions(-)


base-commit: 619ff2fa1d5b60b5fe33216ca2d6219c04a5515b

Comments

Ludovic Courtès Oct. 10, 2023, 1:29 p.m. UTC | #1
Hi,

This looks great to me!

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> +@cindex LGTM, Looks Good To Me
> +@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.

I’d add a few guidelines here, and perhaps we can make it a bullet
list:

  1. 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.

  2. Remain focused: do not change the scope of the work being reviewed.
     For example, if the contribution touches a 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 an problem unrelated to the patch at hand was already there, do
     not ask the submitter to fix it.

  3. 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.

  4. 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.

  5. 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.

I just made it up but I’m sure there are good review guidelines out
there that we could use as an example.

My 2¢,
Ludo’.
Maxim Cournoyer Oct. 12, 2023, 2:51 a.m. UTC | #2
Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> While updating orcus to its latest version (0.19.0), I stumbled on a test a
> failure, which ended up being caused by the lack of test files in the
> repository checkout.  These files are stored using the LFS extension in the
> repo; when not using LFS, empty text stubs with some metadata are left in
> their place.
>
> I've opted to keep the dependency on git-lfs optional for now for the daemon.
> The git in-band downloader will only add the git-lfs dependency when the
> git-reference object has its lfs? field set to true.
>
> Maxim Cournoyer (2):
>   gnu: git-lfs: Patch /bin/sh references.
>   git-download: Add support for Git Large File Storage (LFS).

Apologies for sending this series twice; the first time I sent it
erroneously to 66436 after forgetting to run 'mumi new'.
diff mbox series

Patch

diff --git a/doc/contributing.texi b/doc/contributing.texi
index 864190b119..b09e9cc299 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@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,40 @@  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, building, linting and running other
+people's series and sharing your comments about your experience will
+give some confidence to committers that should result in the proposed
+change being merged faster.
+
+@cindex LGTM, Looks Good To Me
+@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.  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:
+
+@enumerate
+@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 enumerate
+
+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