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

[Maxim Cournoyer]

Hi,

Clément Lassieur <clement@lassieur.org> writes:

> Hi,
>
> These are a few questions regarding reviewing.
>
> 1. What should the reviewer do with old-style patches, like the ones
>    that don't use G-Expressions?  Should we tell the submitter to use
>    them when possible or is it only a matter of style that is up to the
>    submitter?  Obviously they are hard to grasp for newcomers.
>
>    It's probably good for newcomers if we teach them how to use
>    G-Expressions but we don't really have time to do so, given the
>    number of patches waiting to be reviewed.
>
>    This question could be extended to style issues.  Like using %var
>    versus var.

I think we should now make sure all new submissions use the current
style; if they aren't we can demand of the contributors to adjust it.
There is a blog post and enough examples in the code base already that
should make this not too difficult.

> 2. What should the reviewer do when only small changes are required?
>    The reviewer could do these changes in seconds whereas asking for a
>    new revision could take days.  These changes could be indentation
>    fixes, removing of unused code, but they could also be more
>    substantial, like adding a missing `file-name` field.  Or changing
>    old-style to G-Expressions?
>
>    If the reviewer makes such changes and pushes them right away, I
>    imagine they should be documented and explained.

In general, I think it's best to let the contributor know about the
small problems so they can submit a v2, as they'll learn to pay
attention to these.  For first submissions, we can make the experience
easier by adjusting locally and pushing directly for small things.  This
also holds for very old contributions where the original author is no
longer around.

I think these two points could be expound as new subsections of the
'Coding Style' section; the current scope is about codifying the human
interactions more than the technical details.

> 3. Should the reviewer run the program being packaged?  The above
>    guidelines speak about applying, reading, building and linting but
>    not running.  (Making sure it works as expected.)

>From the diff:

--8<---------------cut here---------------start------------->8---
+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.
--8<---------------cut here---------------end--------------->8---

So it does mention trying out the software ("running").

-- 
Thanks,
Maxim