# should I _really_ review trac tickets?

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.
edit retag close merge delete

Sort by » oldest newest most voted

I was trying to answer, and then the wifi on the commuter rail cut out... so I'm retyping it two hours later.

Niles, you should totally review Trac tickets. You are really asking two questions.

1. Should I really review tickets?

2. How much do I need to know in order to do so?

The second question is more at what William and John are saying, and their points are good. But my opinion is that nearly all Sage developers are more likely to err on the side of not reviewing stuff because they feel incompetent/unqualified (mathematically or for the reasons you give) to do so, so I am not so worried about that.

Just don't review those tickets. Review ones where you know there hasn't been a formatting change, or it doesn't affect doctesting (or it fixes one) or whatever. Someone will almost certainly correct you if you are wrong, too, with these relatively simple ones, at least the release manager. Ask someone else to comment (like the others suggest).

But DO the reviews! This is the only way Sage gets better. I think this is really what you were asking. I am certain that starting with small reviews will lead to you getting better and being able to do more. You already know more just from the mistakes you've made than a lot of reviewers. I suppose I also have personal trust that you will be at least as conscientious as I was when I started. You will make mistakes, but that's ok, as it leads to better code long term.

more

1

Yes, I guess the second question is really what I was asking. And I think that having a short but explicit set of guidelines might actually help more people get on with reviewing. As it is, it's too easy to assume that one doesn't have the necessary experience.

( 2010-08-19 23:06:34 +0200 )edit
1

"Simple" fixes like documentation, doctests, and the like are always around and need review. That's how I got started reviewing tickets and it helped me feel more comfortable with the review process. And as William mentioned above, just running a "sage -test" is a great help, pass or fail.

( 2010-08-20 14:55:37 +0200 )edit

I think even a very new developer can definitely review a ticket "negatively", in the sense of pointing out something wrong, and changing the ticket to "needs work". They can also write "I checked all the following and it all looks good to me". They should not change the ticket to "positive review" unless they have a bit more experience and feel "ready".

more

1

thanks, somehow this kind of "middle road" actually hadn't occurred to me

( 2010-08-19 23:07:42 +0200 )edit
1

You could contribute by adding the "middle road" to the official docs somewhere.

( 2010-08-20 04:51:03 +0200 )edit
1
( 2010-08-20 15:17:04 +0200 )edit

William said

They can also write "I checked all the following and it all looks good to me". They should not change the ticket to "positive review" unless they have a bit more experience and feel "ready".

I think that the person could also write, "I would appreciate it if someone more experienced also took a look": they should somehow indicate why they're not giving it a positive review.

(I would have liked to make this a comment in response to William's answer, but there isn't a link for that: I'm only able to comment on my own answers. Is this intentional?)

more