The reviewer's checklist

[nathanncohen]

In the current version of the developer's manual, the section that explains what a reviewer should check is located in the "Sage trac and tickets" section, chapter "The Sage trac server".

http://www.sagemath.org/doc/developer/trac.html#reviewing-patches

What it explains (conventions, documentations, coverage) seems to belong to the 'Basics of Writing and Testing Sage Code' section, and this branch moves it there.

I also rephrased some parts of it, like in tickets #17509 and #17508, so that you can more easily spot what you want to find.

On the way, I also created a section entitled "the status of a ticket" as it seems that we had nothing to explain that.

Nathann

P.S.: The goal is also to have a page we could give to possible contributors, meant to show them the way to review a ticket. For this reason, links are provided toward other sections of the manual, like the git commands, the conventions, etc.

CC: @kcrisman @videlec @sagetrac-tmonteil

Component: documentation

Author: Nathann Cohen

Branch/Commit: 79bbb56

Reviewer: Jeroen Demeyer, Karl-Dieter Crisman, Martin Rubey

Issue created by migration from https://trac.sagemath.org/ticket/17534

[nathanncohen]

New commits:

8cbf24c

trac #17534: The reviewer's checklist

[nathanncohen]

Commit: 8cbf24c

[nathanncohen]

Branch: public/17534

[mantepse]

comment:3

In line 79 of reviewer_checklist.rst I find

**needs_review**: if something is not as it should, write a list of all points

shouldn't that be **needs_work**?

[jdemeyer]

comment:4

You should mention somewhere that a reviewer should add his/her own real name on the Trac ticket.

[jdemeyer]

comment:5

aditional -> additional

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

5ed2df2

trac #17534: Reviewers' comments

[sagetrac-git]

Changed commit from 8cbf24c to 5ed2df2

[nathanncohen]

comment:7

Done !

Nathann

[mantepse]

comment:8

I'm sorry, I missed to mention that needs_review appears erroneously a second time just two lines later.

Martin

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2b0b303

trac #17534: Reviewers' comments

[sagetrac-git]

Changed commit from 5ed2df2 to 2b0b303

[nathanncohen]

comment:10

I'm sorry, I missed to mention that needs_review appears erroneously a second time just two lines later.

It is not exactly like you were the one to make the mistake in the first place :-P

Anyway. Fixed.

Nathann

[kcrisman]

Reviewer: Jeroen Demeyer, Karl-Dieter Crisman, Martin Rubey

[kcrisman]

comment:11

Several comments, though the aim of this ticket is good.

• 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.
• the section 'The Ticket Fields' could use a mention of status (maybe doesn't even have to point below)
• 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... it doesn't have to be long, but we shouldn't make it sound like there is only no code and code ready for review.
• 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).
• 'and the release manager will close it' - if there are no problems like merge conflicts or missed failed doctests or...
• 'needs work' - not just the author, anyone can fix the problems of a needs work ticket
• "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'.
• Under 'speedup' it might be helpful to point out that changes can also slow things down unacceptably, and point to where in the developer manual to find out how to test that.
• 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'.

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.

[nathanncohen]

comment:12

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

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4ae1934	trac #17534: The reviewer's checklist
a4615b0	trac #17534: Reviewers' comments
375d087	trac #17534: Reviewer's comments

[sagetrac-git]

Changed commit from 2b0b303 to 375d087

[kcrisman]

comment:15

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 ?

Unfortunately I won't have time for that for a number of days. All I mean is "Maybe the section 'closing tickets' could become 'reviewing and closing tickets' " and then just add a paragraph saying something like "Tickets can be closed when they have positive review or for other reasons. To learn how to review, please see ." Then the current text follows. I don't agree this is not in scope to at least mention this!

Here again, I do not understand what you mean. The "status" section comes right after the "ticket fields" one.

But nonetheless "status" is a field, so it should be (however briefly) mentioned!

while never boring anyone

That is not one of my primary goals in this particular document, which is different from the intro to the developer guide.

and point to where in the developer manual to find out how to test that.

Where would that be ?

I just assume it exists! Maybe it doesn't.

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 ?

I am talking not about how to write doctests, but how to test and review. In which case we should mention people trying wacky stuff or just lots of ordinary stuff, but I certainly am not suggesting that every such thing should be included as a reviewer patch doctest.

As to the other comments, probably I would write it differently but this is also okay. But I won't be able to finish review until a bit from now, as I said.

[nathanncohen]

comment:16

Yo !

Unfortunately I won't have time for that for a number of days.

Well, I fixed your comments in a new commit, in the hope that you would give this ticket a positive review during your next 10-minutes break :-P

All I mean is "Maybe the section 'closing tickets' could become 'reviewing and closing tickets' " and then just add a paragraph saying something like "Tickets can be closed when they have positive review or for other reasons. To learn how to review, please see ." Then the current text follows.

Done.

But nonetheless "status" is a field, so it should be (however briefly) mentioned!

Done.

That is not one of my primary goals in this particular document, which is different from the intro to the developer guide.

Well, just in case some guy who read the intro would end up on this page, I would also like to make it enjoyable to some extent. I am trying to give everybody the possibility to follow links if they are interested, and to continue with the main document if they are not.

I am talking not about how to write doctests, but how to test and review. In which case we should mention people trying wacky stuff or just lots of ordinary stuff,

Hmmmmm... I would say that a clear page on the different types of doctests, what they check and how they do it would also be of some help to reviewers. Knowing what people usually test in doctests would tell them what they should pay attention to.

but I certainly am not suggesting that every such thing should be included as a reviewer patch doctest.

Oh. Well, I am glad to read that as I had understood it differently. Glad to know that I don't have to write all that right now in order for this patch to pass :-PPPPP

As to the other comments, probably I would write it differently but this is also okay. But I won't be able to finish review until a bit from now, as I said.

Okay. Well, I will push the commit in a second, then it's your turn.

Nathann

[sagetrac-git]

Changed commit from 375d087 to f9345dc

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

f9345dc

trac 17534: Reviewer's comments

[sagetrac-git]

Changed commit from f9345dc to 4ff64c1

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4ff64c1

trac #17534: Reviewer's comments

[kcrisman]

comment:19

Well, I fixed your comments in a new commit, in the hope that you would give this ticket a positive review during your next 10-minutes break :-P

What time zone are you in right now? I wasn't expecting any response until I signed off...

Hmmmmm... I would say that a clear page on the different types of doctests, what they check and how they do it would also be of some help to reviewers. Knowing what people usually test in doctests would tell them what they should pay attention to.

Sure, not on this ticket, of course.

Okay, I'll bite and push something.

By the way, on a first run-through I get

WARNING: duplicate citation WSblog, other instance in /Users/karl.crisman/Downloads/sage/src/doc/en/developer/trac.rst

but I think that only happens when there is already an existing thing, not necessarily when this would actually get done... what about when one upgrades, though?

[sagetrac-git]

Changed commit from 4ff64c1 to 79bbb56

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

79bbb56

Final English and other fixes

[kcrisman]

comment:21

Okay, this is hopefully it, just confirm it builds and looks good and you're happy with it. If the deal with the duplicate citation needs fixing, you'll have to ask someone else - as it says, one doesn't have to review everything in order to be useful :-)

[nathanncohen]

comment:22

Yooooooo !

What time zone are you in right now? I wasn't expecting any response until I signed off...

Still in India !

https://www.google.fr/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8#q=time%20chennai

By the way, on a first run-through I get

WARNING: duplicate citation WSblog, other instance in /Users/karl.crisman/Downloads/sage/src/doc/en/developer/trac.rst

Yeah, I solve it with a "touch *" in the developer/ folder.

but I think that only happens when there is already an existing thing, not necessarily when this would actually get done... what about when one upgrades, though?

NOoooooo idea O_o

I hope that the doc is rebuilt from scratch ?... This will have happened before that patch.

Nathann

[nathanncohen]

comment:23

Yooooooo !

Okay, this is hopefully it, just confirm it builds and looks good and you're happy with it. If the deal with the duplicate citation needs fixing, you'll have to ask someone else - as it says, one doesn't have to review everything in order to be useful :-)

Builds on my machine, and I have nothing against any of that. Thanks ! ;-)

Nathann

[vbraun]

Changed branch from public/17534 to 79bbb56