Developer's Guide: link to Stein's blog post on reviewing

[johanrosenkilde]

From sage-devel, William Stein announced this blog entry:

http://sagemath.blogspot.com/2010/10/how-to-referee-sage-trac-tickets.html

It would be a shame if new developers of Sage could not easily find it.

Apply:

1. attachment: trac_10201_ref_to_review_blog.patch
2. attachment: trac-10201_reviewer.patch

CC: @sagetrac-mvngu

Component: documentation

Author: Johan Sebastian Rosenkilde Nielsen

Reviewer: Minh Van Nguyen

Merged: sage-4.6.1.alpha1

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

[johanrosenkilde]

Attachment: trac_10201_ref_to_review_blog.patch.gz

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

Description changed:

--- 
+++ 
@@ -1,4 +1,5 @@
-William Stein wrote this blog entry:
+From [sage-devel](https://groups.google.com/group/sage-devel/browse_thread/thread/d83f8d87962f8a9f/), William Stein announced this blog entry:
+
 http://sagemath.blogspot.com/2010/10/how-to-referee-sage-trac-tickets.html

 It would be a shame if new developers of Sage could not easily find it.

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

comment:2

Better yet is to directly add William's writings to the Developer's Guide. That's better than providing a link to his blog post. Hence, I'm moving this ticket to "needs work".

[johanrosenkilde]

comment:4

Replying to @sagetrac-mvngu:

Better yet is to directly add William's writings to the Developer's Guide. That's better than providing a link to his blog post. Hence, I'm moving this ticket to "needs work".

I don't immediately agree. I think that would muddle up the instructions on reviewing too much; we already have two descriptions which are referred to as more or less mandatory reading (the one in the Walkthrough and the one in the Trac manual). I remember how much work it felt like, having to read many pages of guide before being able to review a single patch. Having a third covering almost the same is way too much. That's why I added William's blog as a link, which gives it an air of "this is how someone else does it as well, which you might read now or once you've tried reviewing a few times.".

I think William's blog entry contained three important additions to what is already in the documentation:

• A better explanation of "why should you review"
• A (brief) guide on how William does it, what he considers important and so on, giving a second aspect of Sage and reviewing.
• Ideas on how to automate and improve the process for reviewing many tickets.

So I think the best alternative to linking to the blog entry is to clean up the guide to have one - more comprehensive - description of the reviewing process, compiled from the three we have now. This should contain all the goodies of all three, eatly written into one and structured into how to do it the first time, and how to later streamline the process when reviewing many patches. This would probably require a major restructuring of at least the Walkthrough and the Trac Manual, though.

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

Changed author from jsrn to Johan Sebastian Rosenkilde Nielsen

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

Reviewer: Minh Van Nguyen

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

comment:5

Attachment: trac-10201_reviewer.patch.gz

Replying to @johanrosenkilde:

I don't immediately agree. I think that would muddle up the instructions on reviewing too much; we already have two descriptions which are referred to as more or less mandatory reading (the one in the Walkthrough and the one in the Trac manual). I remember how much work it felt like, having to read many pages of guide before being able to review a single patch.

The walk-through is meant to be what its name suggests: a bird's eye view or overview of the whole Sage development process. The walk-through is designed to provide beginners with an introductory appreciation for how to familiarize themselves with the development process. This means that the walk-through can only present certain information at an introductory level and points out other sources containing detailed information. I have uploaded a patch (on top of yours) that hopefully clarifies this point, i.e. for detailed information on the review process, see the section "Reviewing patches" in the Developer's Guide. Someone other than me needs to look over that patch. I'm OK with your patch. If my patch gets a positive review, then the whole ticket gets a positive review.

Having a third covering almost the same is way too much. That's why I added William's blog as a link, which gives it an air of "this is how someone else does it as well, which you might read now or once you've tried reviewing a few times.".

My intention was not clear. I wanted to merge William's materials into the section "Reviewing patches", not create a new section in the Developer's Guide that contains William's materials.

I think William's blog entry contained three important additions to what is already in the documentation:

• A better explanation of "why should you review"
• A (brief) guide on how William does it, what he considers important and so on, giving a second aspect of Sage and reviewing.
• Ideas on how to automate and improve the process for reviewing many tickets.

So I think the best alternative to linking to the blog entry is to clean up the guide to have one - more comprehensive - description of the reviewing process, compiled from the three we have now. This should contain all the goodies of all three, eatly written into one and structured into how to do it the first time, and how to later streamline the process when reviewing many patches. This would probably require a major restructuring of at least the Walkthrough and the Trac Manual, though.

I agree with your point about putting William's materials into the Developer's Guide; this should be a separate ticket. However, I disagree with your argument that there are currently two separate guidelines in the Developer's Guide for reviewing patches. At best, the materials in the walk-through can only serve as an entry point into other parts of the Developer's Guide where more detailed information is available. Hence, I think the walk-through chapter is required reading for any new contributors. If in the future they want further, detailed information on a particular issue, they should consult the relevant section(s) of the Developer's Guide. Please note that the above comments are made from someone (me) who is more or less (very) familiar with the Developer's Guide. This means that a lot of the time many issues are very clear to me (in my mind). However, it is always a good policy to have new contributors comment on the shortcomings of the Sage standard documentation. I hope my reviewer patch (see attachment: trac-10201_reviewer.patch) has clarified that the section "Reviewing patches" in the Developer's Guide is where anyone should consult for detailed information on the review process.

[johanrosenkilde]

comment:6

Your patch seems very sensible to me. It is hard to judge exactly how someone reading this for the first time -- with all the awesomeness of Sage and the development process looming above him -- will read this, but with the explicit wording of "detailed information", it seems ok.

I can see your point regarding the walk-through, and with fresh eyes, I think actually the Developer's Guide is well structured on this point as it is. An elegant merger of the current Reviewing Patches and William's blog is the way to go in the long run then.

Should this patch just get the green light then? And then at some later point, a Trac will be opened on merging William's blog entry directly and removing the link added by this trac.

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

Description changed:

--- 
+++ 
@@ -3,3 +3,8 @@
 http://sagemath.blogspot.com/2010/10/how-to-referee-sage-trac-tickets.html

 It would be a shame if new developers of Sage could not easily find it.
+
+**Apply:**
+
+1. [attachment: trac_10201_ref_to_review_blog.patch](https://github.com/sagemath/sage-prod/files/10651318/trac_10201_ref_to_review_blog.patch.gz)
+2. [attachment: trac-10201_reviewer.patch](https://github.com/sagemath/sage-prod/files/10651319/trac-10201_reviewer.patch.gz)

[7c09a680-e216-4024-bb8e-9bfd4aa7f313]

comment:7

Replying to @johanrosenkilde:

Should this patch just get the green light then?

I have given the green light to your patch. Since you are reviewing my patch, from your review above I gather that you're happy with my patch. Hence, you need to set the ticket state to positive review.

And then at some later point, a Trac will be opened on merging William's blog entry directly and removing the link added by this trac.

See #10218 for a follow-up ticket. I have changed the subject of the current ticket accordingly.

[jdemeyer]

Merged: sage-4.6.1.alpha1