Re: CoC, inclusivity etc. (was "Re: [...] systemd timers on Linux")

[Jeff King <peff@peff.net>]

On Fri, May 28, 2021 at 10:44:25AM -0400, Phillip Susi wrote:

> >   - temper small corrections with positive feedback. Especially for new
> >     contributors, being told explicitly "yes, what you're trying to do
> >     here overall is welcome, and it all looks good except for this..."
> >     is much more encouraging than "this part is wrong". In the latter,
> >     they're left to guess if anybody even values the rest of the work at
> >     all.
> 
> When I see only a minor nit like that I assume that by default, that
> means there are no more serious issues, fix the typo, and resubmit.  If
> a new contributor thinks that means they aren't welcome then I think
> they have an expectation mismatch.

Sure, that may be your intent as a reviewer. My point was that the
recipient of the review does not always know that. Helping them
understand those expectations is part of welcoming them into the
community.

And even as somebody who has been part of the community for a long time
and understands that, it is still comforting to get actual positive
feedback, rather than an assumed "if I did not complain, it is OK".

> >   - likewise, I think it helps to give feedback on expectations for the
> >     process. Saying explicitly "this looks good; I think with this style
> >     change, it would be ready to get picked up" helps them understand
> >     that the fix will get them across the finish line (as opposed to
> >     just getting another round of fix requests).
> 
> That would be nice, but such comments can really only come from a
> maintainer that plans on pushing the patch.  Most comments come from
> bystanders and so nessesarily only consist of pointing out flaws, and
> don't really need to be bloated with a bunch of fluff.  I prefer short,
> and to the point communication.

Yes, I hesitated a little bit on this advice for that reason: it may be
even worse to mislead people about the state of a patch series, if you
the reviewer and the maintainer do not agree. IMHO it is still good for
reviewers to try to help manage newcomers through the process, but it
does make sense for them to be careful not to over-promise.

> > I would even extend some of those into the code itself. Obviously we
> > don't want to lower the bar and take incorrect code, or even typos in
> > error messages. But I think we could stand to relax sometimes on issues
> > of style or "I would do it like this" (and at the very least, the
> > "temper small corrections" advice may apply).
> 
> Isn't saying "I would do it like this" already a tempering statement?  I
> take that as meaning there isn't anything neccesarily wrong with what
> you did, but you might consider this advice.

Here I more meant cases where the two approaches have about the same
value. If your "I would do it like this" can be backed up with reasons
why it might be better (more efficient, more maintainable, and so on),
then that's probably helpful review to give. If it can't, then I'd
question whether it is worth the time to even bring up.

There's a big gray area, of course. Saying "it would be more readable
like this..." is sometimes a nuisance, and sometimes great advice. One
extra complication is that new contributors are often unsure how strong
the request is (e.g., if they disagree, do they _need_ to change it for
the patch to be accepted, or is it OK). I'll often qualify comments with
an explicit "I'm OK with doing it this way, but in case you really like this
other direction, I thought I'd mention it...".

Another complication with all of this advice is that sometimes new
contributors are in a mentoring relationship with one or more reviewers
(e.g., GSoC, Outreachy, or just people who have asked for help). And
there the cost/benefit tradeoff is different between frustrating a new
contributor and teaching them our style, norms, etc.

-Peff