Opened 5 years ago

Closed 5 years ago

#17534 closed enhancement (fixed)

The reviewer's checklist

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.5
Component: documentation Keywords:
Cc: kcrisman, vdelecroix, tmonteil Merged in:
Authors: Nathann Cohen Reviewers: Jeroen Demeyer, Karl-Dieter Crisman, Martin Rubey
Report Upstream: N/A Work issues:
Branch: 79bbb56 (Commits) Commit: 79bbb562a3707c0a782666196ad879f7a33545c6
Dependencies: Stopgaps:

Description

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.

Change History (24)

comment:1 Changed 5 years ago by ncohen

  • Branch set to public/17534
  • Commit set to 8cbf24c7d0675099737057dbff9f9445228fb5a4
  • Status changed from new to needs_review

New commits:

8cbf24ctrac #17534: The reviewer's checklist

comment:2 Changed 5 years ago by ncohen

  • Cc vdelecroix tmonteil added

comment:3 Changed 5 years ago by mantepse

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**?

comment:4 Changed 5 years ago by jdemeyer

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

comment:5 Changed 5 years ago by jdemeyer

aditional -> additional

comment:6 Changed 5 years ago by git

  • Commit changed from 8cbf24c7d0675099737057dbff9f9445228fb5a4 to 5ed2df25c734c443485bf8ef959c0404cd77110a

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

5ed2df2trac #17534: Reviewers' comments

comment:7 Changed 5 years ago by ncohen

Done !

Nathann

comment:8 follow-up: Changed 5 years ago by mantepse

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

Martin

comment:9 Changed 5 years ago by git

  • Commit changed from 5ed2df25c734c443485bf8ef959c0404cd77110a to 2b0b303b315c678d1f58a9456d83fe30b9cf3fb1

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

2b0b303trac #17534: Reviewers' comments

comment:10 in reply to: ↑ 8 Changed 5 years ago by ncohen

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

comment:11 follow-up: Changed 5 years ago by kcrisman

  • Reviewers set to Jeroen Demeyer, Karl-Dieter Crisman, Martin Rubey
  • Status changed from needs_review to needs_work

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.

comment:12 in reply to: ↑ 11 Changed 5 years ago by ncohen

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

comment:13 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:14 Changed 5 years ago by git

  • Commit changed from 2b0b303b315c678d1f58a9456d83fe30b9cf3fb1 to 375d087b7820996f237ff97d49b971e1e921194a

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

4ae1934trac #17534: The reviewer's checklist
a4615b0trac #17534: Reviewers' comments
375d087trac #17534: Reviewer's comments

comment:15 follow-up: Changed 5 years ago by kcrisman

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 <the following section>." 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.

comment:16 in reply to: ↑ 15 Changed 5 years ago by ncohen

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 <the following section>." 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

comment:17 Changed 5 years ago by git

  • Commit changed from 375d087b7820996f237ff97d49b971e1e921194a to f9345dcbacd092410215b19414b827bda6f85e04

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

f9345dctrac 17534: Reviewer's comments

comment:18 Changed 5 years ago by git

  • Commit changed from f9345dcbacd092410215b19414b827bda6f85e04 to 4ff64c1ef8d6d03622c01db321ef76807b6adfef

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

4ff64c1trac #17534: Reviewer's comments

comment:19 Changed 5 years ago by kcrisman

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?

comment:20 Changed 5 years ago by git

  • Commit changed from 4ff64c1ef8d6d03622c01db321ef76807b6adfef to 79bbb562a3707c0a782666196ad879f7a33545c6

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

79bbb56Final English and other fixes

comment:21 follow-up: Changed 5 years ago by kcrisman

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

comment:22 Changed 5 years ago by ncohen

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

Last edited 5 years ago by ncohen (previous) (diff)

comment:23 in reply to: ↑ 21 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

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

comment:24 Changed 5 years ago by vbraun

  • Branch changed from public/17534 to 79bbb562a3707c0a782666196ad879f7a33545c6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.