On Sat, 7 Jul 2018 at 22:45, Glyph <glyph@twistedmatrix.com> wrote:
On Jul 7, 2018, at 12:36 PM, Tom Most <twm@freecog.net> wrote:
[snip]
Thanks for driving this to completion, Tom! Much appreciated.
For future reference though, when landing stuff on trunk, please use this format exactly:
https://twistedmatrix.com/trac/wiki/ReviewProcess#Authors:Howtomergethechang...
This helps with both automatically closing the associated Trac ticket (I already did that in this case) and properly attributing credit for reviewer points on https://twistedmatrix.com/highscores/.
I always forget the format and then I have to search previous commits or wiki. We make reference to the Trac ticket in PR title, Branch Name, PR description (As part of checklist). Do we really need to make another reference in the merge commit? For me, it's red tape. I don't think that we should make things more complicated, just to have a functional https://twistedmatrix.com/highscores/
Perhaps we should avoid the "cleared to land" label on PRs from non-committers? I scan through open PRs for ones which require a procedural nudge now and again, but I had not looked at this one as the process appeared to be done with it, and I missed that it was from a non-committer.
Ideally it would be used sparingly, but, the availability of such a process release-valve allows someone to do a review even if they only have time to read the code, and not the time to sit around waiting 40 minutes for some intermittent CI nonsense to shake out.
Since this doesn't have a https://twisted.reviews/ -like "core gameplay loop" that project members regularly visit, perhaps if you're going to use this label you should always shoot a message to this list as well, to let other contributors know that they should take a look if they have a minute?
On that note, it looks like https://github.com/twisted/twisted/pull/1010 has suffered the same fate? Any other committers have a minute to land that one? :)
https://github.com/twisted/twisted/pull/1010 was merged now. Do we really need and up to date branch before a merge? I was thinking that with the "clear to land" label, we can have a robot which checks for the PR and once all tests are green, the robot will automatically merge based on https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button The robot can also auto-update with 'trunk/master' a PR with clear to land. I am happy to work at automation and improving the Twisted PR process. For example, I have this PR https://github.com/twisted/twisted/pull/1011 It only touches .circleci/config.yml .... why should we block or wait for the merge/review of this PR due to random failures on Buildbot? ----- BTW. Do we need to run the documentation tests on Buildbot? There are some documentation tests on Circle-CI. Is that not enough ? https://circleci.com/gh/twisted/twisted/1215 -- Adi Roiban