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’.
next prev parent 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).