The reviewer’s check list¶
All code that goes into Sage is peer-reviewed. Two reasons for this are:
- Because a developer cannot think of everything at once;
- Because a fresh pair of eyes may spot a mathematical error, a corner-case in the code, insufficient documentation, a missing consistency check, etc.
Anybody (e.g. you) can do this job for somebody else’s ticket. This document lists things that the reviewer must check before deciding that a ticket is ready for inclusion into Sage.
- Do you know what the trac server is? If not, click here.
- Do you have a trac account? If not, click here.
You can now begin the review by reading the diff code.
Read the diff: the diff (i.e. the ticket’s content) can be obtained by
clicking on the (green) branch’s name that appears on the trac ticket. If that
name appears in red (see The Ticket Fields) you can say so in a comment
and set the ticket to
needs_work (see The status of a ticket).
The following should generally be checked while reading and testing the code:
- The purpose: Does the code address the ticket’s stated aim? Can it introduce any new problems? Does testing the new or fixed functionality with a variety of input, not just the examples in the documentation, give expected and robust output (and no unexpected errors or crashes)?
- User documentation: Is the use of the new code clear to a user? Are all mathematical notions involved standard, or is there explanation (or a link to one) provided? Can he/she find the new code easily if he/she needs it?
- Code documentation: Is the code sufficiently commented so that a developer does not have to wonder what exactly it does?
- Conventions: Does the code respect Sage’s conventions? Python’s convention? Cython’s convention?
- Doctest coverage: Do all functions contain doctests? Use
sage -coverage <files>to check it. Are all aspects of the new/modified methods and classes tested (see Writing Testable Examples)?
- Bugfixes: If the ticket contains a bugfix, does it add a doctest
illustrating that the bug has been fixed? This new doctest should contain the
ticket number, for example
- Speedup: Can the ticket make any existing code slower? if the ticket claims to speed up some computation, does the ticket contain code examples to illustrate the claim? The ticket should explain how the speedup is achieved.
- Manuals: Does the reference manual build without errors (check both html and pdf)? See The Sage Manuals to learn how to build the manuals.
- Run the tests: Do all doctests pass without errors? Unrelated components
of Sage may be affected by the change. Check all tests in the whole library,
including “long” doctests (this can be done with
make ptestlong) and any optional doctests related to the functionality. See Running Sage’s doctests for more information.
You are now ready to change the ticket’s status (see The status of a ticket):
- positive review: If the answers to the questions above and other
reasonable questions are “yes”, you can set the ticket to
positive_review. Add your full name to the “reviewer” field (see The Ticket Fields).
- needs_work: If something is not as it should, write a list of all points
that need to be addressed in a comment and change the ticket’s status to
- needs_info: If something is not clear to you and prevents you from going
further with the review, ask your question and set the ticket’s status to
- If you do not know what to do, for instance if you don’t feel experienced enough to take a final decision, explain what you already did in a comment and ask if someone else could take a look.
Reviewer’s commit: if you can fix the issues yourself, you may make a commit in your own name and mark the commit as a reviewer’s patch. To learn how click here (git trac) or here (git only). This contribution must also be reviewed, for example by the author of the original patch.
For more advice on reviewing, see [WSblog].
“The perfect is the enemy of the good”
The point of the review is to ensure that the Sage code guidelines are followed and that the implementation is mathematically correct. Please refrain from additional feature requests or open-ended discussion about alternative implementations. If you want the patch written differently, your suggestion should be a clear and actionable request.
|[WSblog]||William Stein, How to Referee Sage Trac Tickets, http://sagemath.blogspot.com/2010/10/how-to-referee-sage-trac-tickets.html (Caveat: mercurial was replaced with git)|