#17534: The reviewer's checklist
-------------------------+-------------------------------------------------
Reporter: | Owner:
ncohen | Status: needs_work
Type: | Milestone: sage-6.5
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers: Jeroen Demeyer, Karl-Dieter
documentation | Crisman, Martin Rubey
Keywords: | Work issues:
Authors: | Commit:
Nathann Cohen | 2b0b303b315c678d1f58a9456d83fe30b9cf3fb1
Report Upstream: N/A | Stopgaps:
Branch: |
public/17534 |
Dependencies: |
-------------------------+-------------------------------------------------
Changes (by kcrisman):
* status: needs_review => needs_work
* reviewer: => Jeroen Demeyer, Karl-Dieter Crisman, Martin Rubey
Comment:
Several comments, though the aim of this ticket is good.
* You completely removed the 'reviewing patches' section, but it does seem
good to have some mention of this. Maybe the section 'closing tickets'
could become 'reviewing and closing tickets' and have a brief paragraph
encourage review but pointing to the new document, and then the current
language about closing.
* the section 'The Ticket Fields' could use a mention of status (maybe
doesn't even have to point below)
* The whole bit about 'new' tickets is really totally not true. 'new'
means anything that isn't 'needs review'. You should mention that often
'new' tickets do have partial code that someone didn't finish, or may be
able to have discussion about how to approach a problem or new
functionality... it doesn't have to be long, but we shouldn't make it
sound like there is only no code and code ready for review.
* along those lines, you could say that if no one else is working on a
ticket yet, you can say you are planning to, rather than asking whether
anyone else is (and then likely getting no response for months).
* 'and the release manager will close it' - if there are no problems like
merge conflicts or missed failed doctests or...
* 'needs work' - not just the author, anyone can fix the problems of a
needs work ticket
* "If that name appears in red you can say so in a comment" - you should
explain why that is bad, as it may not be obvious to a newbie, especially
if they are not familiar with 'merges'.
* Under 'speedup' it might be helpful to point out that changes can also
slow things down unacceptably, and point to where in the developer manual
to find out how to test that.
* Perhaps most importantly, there is no mention in the old or new
guidelines of "stress-testing". It is vital in testing a change to try
unexpected input - not garbage input, at least not necessarily (though
David Kirkby would disagree on this, and it's not a waste of time per se)
- and see what happens. To try to see what will happen with 'random'
input. Just to try examples other than the ones mentioned in the ticket
itself! Naturally there is no hard and fast rule on this, but something
like this should be one of the bullet points - review is more than just
checking code and whether it 'works'.
Finally, there are some slight infelicities in the English, but
unfortunately I don't have time this morning to do a detailed review of
that. Things like "This, because", "an insufficient" (just "insufficient
is correct), and the like. But don't worry about it now. It will be
really nice to have a specific place to point people, because this is
asked quite often.
--
Ticket URL: <http://trac.sagemath.org/ticket/17534#comment:11>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
