[Twisted-Python] overview: new review queue venue

[Jonathan Vanasco]

On May 24, 2016, at 6:49 PM, Glyph wrote:

> I KNOW, RIGHT!!!  However, protected statuses somewhat reduce the potential race-condition here.  And contributing to a couple dozen Github projects I have to say that practically this has never been an issue, even though I find it aesthetically appalling, design-wise.

> But sometimes people make a fork and their fork branch just happens to be called 'patch-1', or 'master',  this doesn't really affect our workflow as long as they observe proper PR etiquette and don't push any unrelated revisions to that branch in the meanwhile.

In terms of "git flow" i didn't mean the exact names, just the concept that every fix has it's own dedicated branch. This is something that almost every github-experienced person does automatically (and all current twisted PRs do).  People with less experience on github itself (not a given package) will often just edit the "master" branch and submit.  then they'll forget they did that and edit something else... because of that github peculiarity, and that everything is acting "centralized" on github and not under a "decentralized" model where a particular commit was queued that ends up in the review.  those types of changes can also end up triggering CI tests, which complicates things further.

twisted contributors are likely a bit more experienced and it's a self-selecting pool... i've just seen an increasing number of projects require a dedicated branch and politely reject + ask for anything against 'master' to be resubmitted via a dedicated branch.

> I don't see how it's incompatible.  It seems perfectly fine, as long as we shift from 'review queue == report 25' to 'review queue == open PRs with non-failing status'.

ah, I did not know that was possible.  this ties into me not understanding the content of the queue being PR only (not "all tickets").

>> At least on the github side, using some sort of label to state where in the process (ie, what type of review) something is under would help.
> 
> What do you mean it "would help"?  What problem would it solve?

This would help non-maintainers understand what that actual progress was in that 5 stage review.

> 
>> The caveat is that this breaks the current use of Issue as a "queue" item.  But Twisted is using "issues" less as an Issue and more as a queue item, while github "Issues" are less of a queue item and amore of an actual issue.  This is a bit confusing and different to how many people will use github.
> 
> I don't know where you're getting this from.  Only tickets in review are queue items.  I have no idea what "actual issue" means here.

Ok. From here to the end I believe I understand my confusion.  The "queue" looked to me like it was "All Trac Tickets" not just "Certain types of Trac Tickets".   I was looking at a few reports and some raw views that showed track tickets that were both bug reports and attempts to fix a specific problem.  It looked like the normal use of github would have run antagonistic to your workflow , but his all seems fine now.

> The practical issue I'm trying to address by switching to github PRs as opposed to trac tickets with the 'review' keyword is the speed and ease of submitting a change and getting CI feedback on that change, in addition to lowering training overhead since many people are familiar with github.  Faster feedback = happier contributors = more maintenance effort productively expended.  The issue I'm trying to address with the slightly odd close-a-PR-to-signify-finished-review workflow is to avoid the common problem of reviewers not knowing which things to review and therefore leaving certain contributions mouldering until their submitters lose interest.  Beyond that, I don't see that we have substantial workflow issues that need solving.

I was mostly concerned with the proposed implementation creating workflow issues.




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