Revision history [back]

As a new developer to sage, it's not clear when a person should start reviewing patches. Perhaps that's intentional, since perhaps different people are ready at different times, but some guidelines would be helpful. For example, "wait until you have at least a couple of positively reviewed patches", or "despite the great need for patch reviews, developers active for less than one year are not expected to review patches". Do either of these really make sense?

The sage developer guide simply says:

If someone (other than you) has posted a patch for a ticket on the trac server, you can review it.

And then proceeds to give instructions.

About the instructions: There are two lists of instructions for patch reviewers, with slightly different advice; the short one links to the long one, but not vice versa, and I don't think the content of the short one is exactly contained in the long one.

short: Walking Through the Development Process: reviewing a patch

long: The Sage Trac Server: reviewing patches

Here are some further issues not addressed by either set of instructions:

• conforming to sage/python coding conventions: how thorough must the reviewer's knowledge be? (am I really expected to know and enforce _all_ of the python coding conventions, for example)
• author/contributor names should be added to source (I thought I saw this somewhere else in the developer guide)
• ticket number should be mentioned in comments (I learned this from a reviewer of one of my patches)
• make ptestlong must pass all tests for the patch to be included; make ptest is not sufficient in general. (learned this the hard way)
• the #random tag is no longer (never was?) necessary, even though you see it in some sage documentation; the random seed is always the same when doctesting, so you really have to implement a random test suite if you want randomized tests.

As a new developer to sage, it's not clear when a person should start reviewing patches. Perhaps that's intentional, since perhaps different people are ready at different times, but some guidelines would be helpful. For example, "wait until you have at least a couple of positively reviewed patches", or "despite the great need for patch reviews, developers active for less than one year are not expected to review patches". Do either of these really make sense?

The sage developer guide simply says:

If someone (other than you) has posted a patch for a ticket on the trac server, you can review it.

And then proceeds to give instructions.

About the instructions: There are two lists of instructions for patch reviewers, with slightly different advice; the short one links to the long one, but not vice versa, and I don't think the content of the short one is exactly contained in the long one.

short: Walking Through the Development Process: reviewing a patch

long: The Sage Trac Server: reviewing patches

Here are some further issues not addressed by either set of instructions:

• conforming to sage/python coding conventions: how thorough must the reviewer's knowledge be? (am I really expected to know and enforce _all_ of the python coding conventions, for example)
• author/contributor names should be added to source (I thought I saw this somewhere else in the developer guide)
• ticket number should be mentioned in comments (I learned this from a reviewer of one of my patches)
• make ptestlong must pass all tests for the patch to be included; make ptest is not sufficient in general. (learned this the hard way)
• the #random tag is no longer (never was?) necessary, even though you see it in some sage documentation; the random seed is always the same when doctesting, so you really have to implement a random test suite if you want randomized tests.