[Twisted-Python] overview: new review queue venue

[Jonathan Vanasco]

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

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.

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

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

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.

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


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.  

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.