On 17/09/13 19:08, Glyph wrote:
On Sep 17, 2013, at 10:43 AM, Phil Mayers <p.may...@imperial.ac.uk
<mailto:p.may...@imperial.ac.uk>> wrote:
On 17/09/13 17:05, exar...@twistedmatrix.com
<mailto:exar...@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)
_______________________________________________
Twisted-Python mailing list
Twisted-Python@twistedmatrix.com
http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
