Re: Dealing with upstream issues

[Ludovic Courtès]

Hi!

Just to be clear, I started this thread as discussion on the kind of
interaction we reviewers should offer to submitters.  I am not
suggesting changes in our “quality standards”, nor am I suggesting that
one group of people do more work.

Maxime Devos <maximedevos@telenet.be> skribis:

> Ludovic Courtès schreef op ma 27-06-2022 om 12:10 [+0200]:
>> Regarding the review process, I think we should strive for a predictable
>> process—not requesting work from the submitter beyond what they can
>> expect.  Submitters can be expected to follow the written rules¹ and
>> perhaps a few more rules (for example, I don’t think we’ve documented
>> the fact that #:tests? #f is a last resort and should come with a
>> comment). 
>> 
>> However, following that principle, we reviewers cannot
>> reasonably ask for work beyond that. [...]
>
> We can ask to do send the notice upstream, if it's in the rules (I
> consider this to be part of the unwritten rules).

Yes, that’s a reasonable thing to ask for.  However, we can only ask for
it if the submitter identified a problem themselves; if I’m the one
finding a problem, it’s inappropriate to ask someone else to report it
on my behalf.

> And I don't often have time for addressing the noticed issues and I
> have other things to do as well -- I usually limit myself to just
> reviewing.  I do not intend to take up work beyond that.

Of course, and I don’t mean reviewers should do more work!  I think the
few people that dedicate time to patch review already have more than
enough on their plate; the last thing I’d want is to put more pressure
on them.  We have to care for one another, and that starts by making
sure none of us feels a pressure to always do more.

> I also consider them to not be rules as in ‘if they are followed, it
> WILL be accepted’ but more guidelines like ‘these things are important,
> they usually need to be followed, but it's not exhaustive, at times it
> might be discovered the list is incomplete’.

Agreed.

> I don't think that patch submitters can reasonably expect reviewers to
> do all the work.

Agreed.

> Also, previously in discussions about the review process, weren't there
> points about a reviewer not having to do everything all at once, they
> could choose to review parts they know how to review and have time for
> and leave the rest for others?

I don’t remember discussions along these lines.  I think it can be
helpful sometimes, and tricky other times.

For example, for a package, I find it hard to split review work.  But
for a patch series that touches many different things (documentation,
build system, importer, whatever), splitting review work among different
people may work better.

>> Related to that, I think it’s important to have a consistent review
>> process.  In other words, we should be equally demanding for all
>> patches to avoid bad surprises or a feeling of double standard.
>
> I think I've been consistent in asking to inform upstream of the issues
> (*), with no distinction of whether it's a new submitter or an
> established one or whatever.

I think our standards should be the same whether the submitter is a
newcomer or not.

The difference is in how we reviewers reply.  To a newcomer, reviewers
should IMO do the “last kilometer” themselves on behalf of submitters:
tweaking the commit log, synopsis/description, indentation, that kind of
thing.  It’s important because that gives submitters a good experience,
it helps them learn, and it’s also low-friction for the reviewer.
(That’s also the whole point of guix-mentors.)

Naturally, one can be more demanding of seasoned contributors, and I
think it’s OK to leave it up to them to fix such things.

Thanks,
Ludo’.

PS: Sorry for the wall of text!