[Twisted-Python] Twisted at PyConUK 2013

[Phil Mayers]

On 17/09/13 19:08, Glyph wrote:
> On Sep 17, 2013, at 10:43 AM, Phil Mayers <p.mayers at imperial.ac.uk
> <mailto:p.mayers at imperial.ac.uk>> wrote:
>
>> On 17/09/13 17:05, exarkun at twistedmatrix.com
>> <mailto:exarkun at twistedmatrix.com> wrote:
>>
>>>> p.s. the "How to review" docs on Trac are AWFUL if you've never done
>>>> one before. It assumes a *hell* of a lot of prior knowledge. There
>>>> needs to be a single page checklist for first-time reviewers.
>>>
>>> This is in progress.
>>
>> Awesome; would it be useful for me to write up what I did, or do you
>> have enough source material?
>
> Let's have a discussion here on the list first :-).  So... yes, write it
> up in a reply.
>
> The review docs are always in progress.  Feedback like "this is bad" is
> basically useless; we know it's bad, but everyone has its own idea of
> what "bad" means.  What would be really useful in such a write-up was
> specific feedback about what you needed to know, what resources you

Yeah, sorry - not great feedback there.

In brief: Twisted reviews are obviously very comprehensive, encompassing 
test passed and coverage, coding style, api design and documentation as 
well as subjective opinion.

You need a bunch of tools to get started, and have to do a few 
boilerplate things to get setup, and I think a really *really* basic 
setup doc, followed by a handholding checklist, would form a better 
basis for a first-time reviewer.

So specifics:

The Trac ReviewProcess page starts off well, but loses coherence about 
half-way through - the first 3 sections are about authoring a change, 
then you get a "Reviewers: how to review (see below) and some link-free 
bullet points", then back to "Authors": then the "Details" section which 
starts with "news files" and it's all a bit uncertain who this applies 
to on a first pass - I had to read it several times, which makes people 
feel dumb.

Minor note: the "How to review a change" is missing an obvious link to 
the coding standards.

Suggestion: split this page into 3 pages - Authoring, Reviewing, 
Committing. Have a master page "Twisted overall process" which links to 
the 3 pages. It's valuable for a reviewer to know the authoring process 
- the mindset is helpful - but a "mode change" should be accompanied by 
an actual "input" change.

Suggestion for the "Review" page: this should be:

1. Setting up a review environment - I think this is really important. 
People who know the process can and will do things their own way, but a 
really basic setup like:

   virtualenv twrev
   . twrev/bin/active
   pip install twisted-reviewtools << make this!
   svn checkout blah
   twrev/bin/python blah/setup.py install
   twrev/bin/trial twisted

...would save first-timers some hassle.

2. Checking out the branch to review and getting it in a review-able 
state (what do people do here - run from inside the branch dir? install 
to a virtualenv?)

3. Running basic checks - tests, pyflakes, twistedchecker - and 
interpreting the results. In particular, techniques for ignoring the 
flakess/checker output for unchanged files.

4. Reviewing the diff:
    * How to check docstrings
    * How to evaluate test coverage

5. How to provide feedback - form, tone, avoiding bikeshedding (or not)

Then link to some "best practice" examples of previous tickets - one or 
two that show a good set of review comments - for people to peruse.

Hope this is useful. I probably am overstating how painful it was - I 
did get it done after all, and it'll be easier 2nd time around - but you 
guys keep saying you want more reviewers ;o)