[Twisted-Python] overview: new review queue venue

[Glyph]

> On May 24, 2016, at 2:49 PM, Jonathan Vanasco <twisted-python at 2xlp.com> wrote:
> 
> just a few thoughts:
> 
> The current system as-explained seems to use an "Issue" as a queue item that is either a "bug report" or a "notice of a pull request, which may also reference another bug report".

In the current system, we have "tickets" (in Trac) not "issues" (on Github).  And in that system, an issue is put into review when it has an attached branch or patch that needs to be considered for inclusion.

In the Github system.

> that is weird.

> IMHO, a quality contributor will never get turned off by handling the docs, tests, and other requirements - but may get confused/turned off by this.

Literally everyone's predictions about this have always been wrong, so I don't like speculating ;).  Especially classifying people as "quality contributors" or not.  I will sometimes say something like "if you don't do X, you probably won't do Y" but being a "quality contributor" is a complex, multi-dimensional, and situationally-dependent personality characteristic that I don't think we can predict.

> anyways:
> 
> 1. github's PRs are peculiar in that they're "branch to branch" and not "branch at commit to branch at commit".  If I were to submit a PR, I could still make changes on it between my submission and someone finally doing a merge.  Personally, I find this infuriating.

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.

> In any event, I suggest *requiring* something like the "git flow" model for submissions.  Outright rejecting anything that isn't in a dedicated fix/feature branch.  Everyone is currently doing the right thing, and there are contribution docs that suggest this – I would just make it a stated policy to automatically reject any PRs from someone using "master/trunk".

Honestly I don't think this is worth the effort.  git-flow is, abstractly, a reasonably good idea (if people don't know what we're talking about, see <http://jeffkreeftmeijer.com/2010/why-arent-you-using-git-flow/>) since it encapsulates the precise semantics desired by certain commits.  But sometimes people make a fork and their fork branch just happens to be called 'patch-1', or 'master', but 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.

> 2. regarding Issues vs PRs and working between Trac and Github, I suggest you take the approach of "Trac Issue = Idea" and "Github PR = Implementation of Idea".  Under this concept, one or more PRs might be attempts to address a given Issue.

That's ... exactly what I was trying to express.  (Except I was thinking we'd gradually move from Trac Tickets to Github Issues).

> The core maintainers can address the highlevel concepts and requirements under Issue, while grounds for rejection / feedback on each PR are  listed there.  As a github/bitbucket submitter, it's really useful when someone looks at a diff and uses inline comments to note their rejection for a section.  This is how most people use Github.  (and it's incompatible with the current queue system, I KNOW).

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'.

> 3. Initially I didn't like the idea of rejecting so fast, but after thinking it over, now I do.  In addition to boilerplate text about "please resubmit", I think it might be beneficial to standardize some labels for this though, as it helps future contributors who may want to address an issue.  If the "IDEA" is approved but the implementation is not, then noting "Resubmission Pending" suggests this is still active.  If the underlying IDEA or approach is rejected, then noting "Rejection Final" can suggest it's just not going to happen.

Yeah, adding some informative labels to the closed issues might be a good way of letting contributors know what is going on.  I just wanted to make sure that they are an optional part of the workflow which, if the automation around them breaks down in some way, we can still make progress, and we don't end up with inconsistent or lost information.

> 4.  I understand that "review" hold special significance within the twisted team's workflow, but outside of Twisted it is awkward to see something bounce in and out of "review".

I think this is because, generally speaking, most teams do a poor job structuring their communications about what state issues and pull requests are in :).

> 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?

> 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.

> 
> Just to illustrate what a typical github contributor flow might be:
> 
> 	Scenario A-
> 
> 		There is a trac Issue #1001 for "F is broken"
> 		bob generates github pr 11, and submits.  He comments in #1001
> 		ted generates github pr 12, and submits.  He creates a trac issue #1002.  When #1002 is first looked at, it is a dupe of 1001 and copied over.
> 		carol generates github pr 13 and submits.  she comments in #1003
> 
> 		There are 3 PRs for a single Issue.  Feedback on the PRs occurs on Github.  Feedback on the Ideas and status of PRs is on trac.
> 
> 	Scenario B-
> 		Bob notes that "X is broken"
> 		Bob generates github PR 11 and submits.  He goes to trac and creates issue #1002
> 		Bob's PR is rejected because the fundamental approach is not acceptable, however X is still broken.  PR11 is closed, #1002 is open.
> 		Ted generates pr 12 and submits.  he comments in #1002.  Ted is rejected but the approach is compatible.  it can be implemented with fixes.
> 		Carol forks Ted's approach , fixes it, and generates pr 13.  she comments in #1002 and it is accepted.
> 
> again, this breaks the current use of queues.  I'm not sure how to reconcile everything together, however I think you're going to find non-core maintainers naturally fall into the 2 scenarios above.

What use?  What are you referring to with the plural "queues"?  There's only ever been a single queue, the review queue.  In both of your scenarios, the list of open PRs is the review queue, and in both of those scenarios, discussion of the abstract idea can happen on the tickets without treating them directly as queue items.

I appreciate you taking the time to think about our workflow and the review queue, but I would ask that you carefully consider what problem you're trying to address with your suggestions.

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.

-glyph

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