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).

Build the code: while you read the code, you can rebuild Sage with the new code. If you do not know how to download the code, click here (with git trac) or here (git only).

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 See :trac:`12345`.

  • 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.

  • Build the manuals: Does the reference manual build without errors (check both html and pdf)? See The Sage Manuals to learn how to build the manuals.

  • Look at the manuals: Does the reference manual look okay? The changes may have typos that allow the documentation to build without apparent errors but that may cause badly formatted output or broken hyperlinks.

  • 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_work.

  • 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 needs_info.

  • 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].

Note

“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.

REFERENCES:

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)