#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: |
-------------------------+-------------------------------------------------
Comment (by ncohen):
Hellooooooo Karl-Dieter,
I am sorry that I do not understand the intention behind many of your
remarks, so I cannot make most of those modifications you asked. More
specifically
> * 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.
I removed this section because it explained, in the 'Sage trac and ticket'
chapter, how to check Sage's code which I found out of scope. I do not
understand which kind of mention you would like to have of reviews in this
part of the code: could you add a commit for that ?
I have to say that I would be glad to rename this "closing tickets" to
"Request the cosure of a ticket" or something. Nobody closes tickets
except a guy who will not be learning his job in the developer's manual
`:-P`
> * the section 'The Ticket Fields' could use a mention of status (maybe
doesn't even have to point below)
Here again, I do not understand what you mean. The "status" section comes
right after the "ticket fields" one.
> * 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...
To me you describe first a ticket that should be in 'needs_work' then a
ticket that should be in 'needs_info'. Is that okay with the comment I
added ? It says that 'some tickets are "new" only because nobody thought
of changing it something else'.
> * 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).
Done.
> * 'and the release manager will close it' - if there are no problems
like merge conflicts or missed failed doctests or...
It is true, but I believe that saying that here can only result in
pointless worrying. If they have done the review correctly the only thing
that could make this happen is because some releases are made between
their review and the closing, and we have no control over that as
developpers.
> * 'needs work' - not just the author, anyone can fix the problems of a
needs work ticket
I made this sentence 'anonymous'.
> * "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'.
I feel that this should be explained, but not here. It would be better if
there was a link to point to, some page in the 'trac' section explaining
about what appears on a ticket, including the branch's color, and what
exactly it means. In this document I am already having a hard time trying
to say everything to anybody that might need it while never boring anyone
`^^;`
> * Under 'speedup' it might be helpful to point out that changes can also
slow things down unacceptably,
Totally right. Done.
> and point to where in the developer manual to find out how to test that.
Where would that be ?
> * 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'.
Hmmmm.. We should probably add a section like that in the 'doctest'
section. Actually, we should probably have a page explaining what is a
'good doctest', what it checks, how, the random seed and stuff. Then we
could have a link pointing toward that ?
> 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.
Well, I just pushed my commit so it is your turn, whenever you like.
Off to breakfast, then to my talk `:-P`
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/17534#comment:12>
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.
