[Twisted-Python] overview: new review queue venue

[Glyph]

> On May 21, 2016, at 7:25 PM, Clayton Daley <clayton.daley at gmail.com> wrote:
> 
> To qualify my comments, I've yet to contribute to Twisted because I don't have a good enough grasp of its internals, but I have contributed to a variety of Git-based OSS projects.  I definitely get uneasy with the general idea that we're trying to "replicate workflow A from Trac in tangentially related Git PR feature".

The workflow is not "from Trac".  The instantiation in Trac is not optimal either, which is why I described the abstract desired workflow separately from our existing instantiation.

> We're in Git. We're hoping to solicit PRs from Git users. Git users will be used to the way PRs are used in other OSS Git projects.

I think you mean "GitHub".  Git PRs don't work at all like GitHub PRs. :).

> Glyph has some valid criticisms of the situation in some projects, but it should still be the starting point. For example, closing a PR strikes me as a bad idea -- for lack of a better word, it feels "hostile" to me and certainly unwelcoming.

The thing is, if you perceive it as "hostile" that a project closes a PR - i.e. "says that they're not going to do more work on it" - that is a cultural problem; it suggests a certain implicit level of passive aggression in opening a PR which I don't want to assume.  It's sort of like having a culture where you can just send anybody an email asking them to do whatever and it would be "hostile" for them to refuse.  In such a culture people don't say "yes", but they do start to ignore messages  Closing the PR is a more accurate reflection of reality - the project (twisted) is not going to do anything about the PR in its current state, so it shouldn't be left open.  It also clearly demarcates the completion of a review.

People feel very differently about workflow, of course, but I've definitely heard from other OSS maintainers that the average workflow of volunteer projects often devolves into a huge backlog of un-reviewed stuff, which obscures the new stuff, and if you want something to actually get reviewed and move along you need to know the maintainers of the project and ask them personally.

I'd much rather our new contributors get a little confused about the culturally unusual step of closing a PR than to have their work be accidentally but systematically discriminated against in favor of people who know how to bug the right people in IRC or email.

> In several of the projects I've seen, Git tags fill these roles. Piwik has a "needs review" tag -- the short list for reviewers. Looks like it's a manual add, but maybe it could be automated. Once reviewed, Piwik has tags like "Tests & QA". ZendFramework has a generic "awaiting author update".

This was my original idea. The problem with GitHub labels ("Git tags" are something completely different) is that they can't be applied by external contributors.  You need write access to the repository to be able to manipulate them.  It's very important to our workflow that external contributors be able to re-submit their PRs.  We could have a bot for that (again, this was the original plan).  But it seems like using the open / closed state to reflect the we will do some work on this / we won't do any more work on this is actually closer to the "native" state of github.

> To address Glyph's concerns about lingering PRs, perhaps the combination of:
> A policy like "a reviewer must accept, close, or tag with one of the next step tags"
This doesn't address the shortcomings of labels, to wit, external contributors need the ability to manipulate them somehow, and if a PR isn't "in review" by default, then they have to whisper some magic comment to make anyone take a look at it.  If we use open/closed, then the default action of "open a PR" will cause someone to look at it, even if our bot is nonexistent or temporarily offline.
> A short list of common next steps like "code quality", "needs tests", "second opinion", "not review ready"... plus a generic "other author action"
The next steps are always the same: "respond to the review".  The review may include any of these, any combination of these, or also possibly questions that the author must answer.  Formally separating these would be a weird tweak on our current workflow that I don't see helping.
> Auto-close tickets except those with "needs review" or "second opinion" (say) 30 days after the last action.
I think you mean "PRs", not "tickets"?  Issues (which are closer to trac "tickets") can remain open indefinitely.  I am not really in favor of any kind of auto-closing or expiration given that the project has wildly variable levels of resources depending on people's spare time; sometimes we have a whole bunch of reviewers active and can get to things within 24 hours, sometimes we're severely overtaxed and can't look at anything for 6 weeks.

> Drive-by comments on a PR are sometimes helpful, but should be used sparingly.  Mostly, discussion should happen on issues, not PRs.  A PR is a suggested resolution to a problem, and we might reject one solution, but an issue should describe the problem itself.
> 
> While an Issue is a good place for discussion about a problem, it lacks the reference code often included in a PR.  You can't ask "how about this approach" without showing the approach.  As an added bonus, most systems run travis on PRs so you get a sense of "this approach is thorough" or "this idea still breaks something".

This is exactly why issues and PRs should be separated.  If you only have one artifact - the PR - to represent both the issue and the potential solution, then you can't get rid of the potential solution (reject the PR) without also getting rid of the description of the problem.  Github already automatically shows anywhere that your PR or issue is mentioned, so all you need to do to say "how about this approach" is to put the words "fixes #NNN" in your PR description.  As a bonus, that will make it automatically close the issue when the PR is merged.

-glyph
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160521/bbecf259/attachment-0002.html>