unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: "Ludovic Courtès" <ludo@gnu.org>
To: Maxim Cournoyer <maxim.cournoyer@gmail.com>
Cc: 66436@debbugs.gnu.org
Subject: [bug#66436] [PATCH] doc: Add some guidelines for reviewing.
Date: Tue, 10 Oct 2023 15:29:37 +0200	[thread overview]
Message-ID: <87r0m2wqpq.fsf@gnu.org> (raw)
In-Reply-To: <19f82d9bbef649c750ad067d23ebbaee6f9ae494.1696942467.git.maxim.cournoyer@gmail.com> (Maxim Cournoyer's message of "Tue, 10 Oct 2023 08:54:27 -0400")

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




  reply	other threads:[~2023-10-10 13:31 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-10 12:54 [bug#66436] [PATCH] doc: Add some guidelines for reviewing Maxim Cournoyer
2023-10-10 13:29 ` Ludovic Courtès [this message]
2023-10-11  0:24 ` [bug#66436] [PATCH v2] " Maxim Cournoyer
2023-10-15  9:55   ` Josselin Poiret via Guix-patches via
2023-10-16 14:02     ` Maxim Cournoyer
2023-10-29 14:52       ` Josselin Poiret via Guix-patches via
2023-10-20  8:12   ` Clément Lassieur
2023-10-20 23:01     ` Maxim Cournoyer
2023-10-22 20:03       ` Clément Lassieur
2023-10-23  1:55         ` Maxim Cournoyer
2023-10-24  8:49       ` Simon Tournier
2023-10-24  8:59         ` Simon Tournier
2023-10-31 18:53           ` Maxim Cournoyer
2023-10-31 19:03             ` Simon Tournier
2023-10-24 15:54   ` Ludovic Courtès
2023-10-25 13:53     ` Simon Tournier
2023-10-12  2:48 ` [bug#66436] [PATCH 0/2] Add support for Git Large File Storage (LFS) Maxim Cournoyer
2023-10-12  2:51   ` Maxim Cournoyer
2023-10-31 20:25   ` [bug#66475] [PATCH v2 1/4] git-download: " Maxim Cournoyer
2023-10-31 20:25     ` [bug#66475] [PATCH v2 2/4] gnu: mdds: Update to 2.1.1 Maxim Cournoyer
2023-10-31 20:25     ` [bug#66475] [PATCH v2 3/4] gnu: ixion: Update to 0.19.0 Maxim Cournoyer
2023-10-31 20:25     ` [bug#66475] [PATCH v2 4/4] gnu: orcus: " Maxim Cournoyer
2023-11-05 14:49     ` [bug#66475] [PATCH v2 1/4] git-download: Add support for Git Large File Storage (LFS) Ludovic Courtès
2023-11-07 16:17       ` bug#66475: " Maxim Cournoyer
2023-11-01 19:23 ` [bug#66436] [PATCH v3] doc: Add some guidelines for reviewing Maxim Cournoyer
2023-11-05 14:51   ` Ludovic Courtès
2023-11-07 16:14     ` bug#66436: " Maxim Cournoyer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r0m2wqpq.fsf@gnu.org \
    --to=ludo@gnu.org \
    --cc=66436@debbugs.gnu.org \
    --cc=maxim.cournoyer@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).