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

[Simon Tournier]

Hi,

On Fri, 20 Oct 2023 at 19:01, Maxim Cournoyer <maxim.cournoyer@gmail.com> wrote:

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

> --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").

If LGTM also implies “I run it and it is OK”, then submitter should
provide how to run it.

Otherwise, for what it is worth, I will stop to review stuff that I do
not use myself because reviewing is asking me too much: read some doc
about how to run something that I do not care.

    ( Similarly, if LGTM also implies “I have read the source code and
it is OK”, it appears to me too much. )

Well, “running” and “trying out the software” seems ambiguous.  What
does it mean “run IceCat” or “run Gmsh” or else?

What I like with the proposal is that it makes better defined what are
the expectations behind LGTM.  But what means “running” is not clear for
me.

IMHO, it is worth to clearly state:

 1. what helps the review process:

    « 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. »

 2. what means LGTM, from my understanding: applying, building, linting,
    carefully checking the code that is merged to Guix.

Cheers,
simon