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
