[Twisted-Python] quality of non-member reviews

[Glyph]

Hello Twisted Project Members (i.e.: committers, people with repo:write access),

Whenever you get a code review from a non-member, it's important to always[1] ensure that they remove the 'review' keyword.  This is not really important in and of itself, but it is very important for other reasons, which I will attempt to explain, since I think I may not have properly laid this out sufficient detail before.

The goal of allowing non-project-members (i.e.: external contributors) to review changes in Twisted is to provide a pipeline for folks to become developers of the project; to allow people to gain experience with reviewing, so they can proceed to being full reviewers, without requiring that they already be project members before they can practice.  In other words, it is a process to train reviewers so that we can have higher code-review throughput in general in the future, not a shortcut to get changes reviewed more quickly.

It is therefore the responsibility of the developer accepting the code review to ensure that the reviewer is following directions and has given sufficient attention to the review, and to provide feedback on those reviews to ensure that the new contributors are learning how to look for the things that help us maintain Twisted's quality.

Sometimes, the only code review you need is a "thumbs up, looks good", because everything really is fine, the fix is simple, and there's nothing much the reviewer could say.  Ideally, all changes are small, and obvious, and reviewers can just give that review regularly.

However, such a review is always suspicious, because it's very easy for a reviewer, especially an external reviewer with little understanding of the project, to just rubber-stamp a fix they want to see merged quickly without giving it any real analysis.

As such, it's very important that when accepting a very short review that you use whatever clues you have at your disposal that  the reviewer has actually reviewed the code in some detail.

One potential proxy for this is that our process requires some small peculiarities, such as attaching and removing the 'review' keyword in Trac[2].  If a contributor has not even read the instructions carefully enough to know that they need to remove this keyword, it is virtually impossible for them to have dedicated the attention and care required by https://twistedmatrix.com/trac/wiki/ReviewProcess#Howtobeagoodreviewer <https://twistedmatrix.com/trac/wiki/ReviewProcess#Howtobeagoodreviewer> to do a full review and consider everything that cannot be automatically verified, such as impact to Twisted's public interface, the legibility of documentation, duplication of features, et cetera.

If the contributor has demonstrated their attention to detail in some other way, by giving a thorough review and writing some analysis of the change's implications (even if it's not actionable, just something like "I read over the narrative documentation to ensure it didn't need any changes and it looks like we're good") then it might be acceptable to gently point out the small process mistake of not updating trac.  But if the change is just a Github comment saying "LGTM" with nothing else associated, it is important not to accept the review and to explain that the reviewer should re-read the directions explaining how to give a review.

As you might have guessed, I have seen "LGTM" changes landing on trunk recently whose reviews looked somewhat suspicious to me.  I, and a few other long-time Twisted developers, have complained about this before, but in re-reading those complaints in preparation for writing this message, it occurred to me that I may not have spelled out exactly what I was concerned with and why, so, in the spirit if blameless post-mortems, I don't want to call out anyone specifically[3].

Pascal's apology applies; I haven't had much time to edit.

Thanks for reading,

-glyph


Endnotes:

[1]: Iron-clad process rigidity is not the goal here. Sometimes there are legitimate reasons to need to move things along a little faster than a full thorough review requires.  For example, if there's a pressing security issue, or all the CI infrastructure is failing due to a bug in one of our dependencies.  There are also cases where the important thing that needs to be done in a review is a highly specialized analysis, like; for example, if we need a GPS expert to verify that some change to twisted.positioning is correct, and beyond the domain knowledge the code change is otherwise trivial.  In these cases it might be fine to accept a "LGTM", if the question posed in the review is highly specific, but in that case, it's important to leave a comment explaining what exactly has transpired: "I'm removing the review keyword myself because I needed a domain expert to review this, and XXX YYY was helpful enough to have a look but is not familiar with our process." or whatever the case may be.

In general, putting more thorough comments on tickets is always a good thing.  However clear that you as a Twisted project insider think a thing is, chances are it is about 1/4 that clear to anyone trying to understand project direction from the outside.  I'm certainly guilty of under-documenting myself :-).

[2]: we are not preserving Trac purely as a shibboleth to check on people installing the directions; software development is hard enough without extra roadblocks thrown in the way.  There are just too many tools connected to the 'review' keyword right now that are hard to connect to Github automatically.  Whatever our process, however, it's important to verify that reviewers are following it carefully.

[3]: I realize that there are too many bits of identifying information here to be meaningfully anonymous, so some folks will immediately know the specific ticket that precipitated this, but don't be a jerk and dig up someone's name from the trac timeline because you can; the goal here is not (and should not) be to throw anyone under the bus, but to improve communication around project policy.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20170723/24fca52b/attachment.html>