Leif Leonhardy has done a *huge* amount to improve the quality of Sage. I think
if more authors and reviewers were like Leif, we would not have many of the
quality control issues we have in Sage today.
I personally think it is quite reasonable to suggest an author make some changes
to a package outside what they intended, if those changes are relatively small
and improve the package. I did this on
http://trac.sagemath.org/sage_trac/ticket/9766,
My first point was clearly an error that the author needed changing - subsequent
points were trivial and improved the whole package. But after I'd positively
reviewed it, Mitesh found another problem, which I promptly fixed, as the
original author (Nathann Cohen) was backpacking.
Making small additional changes seems quite reasonable to me. If someone updates
a package and should obviously spot some other minor problems, it does not seem
unreasonable they address them.
But this can be taken too far in my opinion.
http://trac.sagemath.org/sage_trac/ticket/9603
was such a ticket, where I feel Leif's requested changes have gone too far.
Despite the current title (which has changed because of the scope of the changes
made), the original patch was something very small, to force the package to
build on HP-UX. This is the original patch. It is very small, and very safe.
http://trac.sagemath.org/sage_trac/attachment/ticket/9603/9603.patch
Leif pointed out that one line still printed a message about iconv only being
installed on Solaris and Cygwin, so omitting HP-UX. I'd agree, that was very
reasonable, so the correction was made.
http://trac.sagemath.org/sage_trac/attachment/ticket/9603/9603-informative-message-correction.patch
But then things seem to have got a bit out of hand in my opinion. Leif suggested
I rewrite the order of the tests. This is really a style issue, but I did that.
http://trac.sagemath.org/sage_trac/attachment/ticket/9603/9603-cleanup.patch
I was not overly happy, as these changes increased the risk of errors creeping
in, as humans make errors. But I made those changes.
Then more changes were requested - some pretty trivial, but again I made them
http://trac.sagemath.org/sage_trac/attachment/ticket/9603/9306-more-cleanup.patch
Then it was suggested {{{make}}} was changed to {{{$MAKE}}} - which I agree is a
good thing, but is in fact in direct contravention to the
[http://www.sagemath.org/doc/developer/producing_spkgs.html#creating-a-new-spkg
Sage Developer's Guide] where it says:
"Ensure that make install is non-parallel, i.e. do export MAKE=make"
Changing to parallel make introduces risks, so I ended up spending a lot lot of
time testing this on multiple platforms. I think its been tested 150 times now.
The gains are quite modest though. I think it saves a about 10 seconds of build
time on my computer, as iconv is a very small package.
Then I found a small Solaris specific issue, so added a patch for that 9 days
ago.
http://trac.sagemath.org/sage_trac/attachment/ticket/9603/9603-move-from-CFLAGS-to-CC.patch
So far the ticket has been open 6 weeks now, does not yet have a positive
review. IMHO, all changes related to the original purpose of the ticket (build
on HP-UX) were resolved more than a month ago.
With the exception of the small Solaris specific issue raised 9 days ago, the
rest are nice, but not necessary. In particular, rewriting 90% of the way the
package was installed, which was really a style issue rather than for any
technical gain, seemed a bit excessive.
Do we have any guidelines on these sorts of issues?
If not, I'd propose that any changes outside the original scope of the ticket
that will take the author less than 30 minutes to address, would be quite
reasonable.
But if the changes are going to take more than 30 minutes, the reviewer should
put them on another ticket.
It was clear to me in #9603 that some of the changes proposed were small,
desirable and quick to do. Other changes, like rewriting the whole test in a way
that more more a matter of style than anything else, and enabling parallel
builds, were far more risky changes.
The ticket, which was originally to add a few bytes to install iconv has now
taken up several days of my time, and it still has no positive review after 6
weeks, and has no comments for 9 days.
Dave
--
To post to this group, send an email to sage-devel@googlegroups.com
To unsubscribe from this group, send an email to
sage-devel+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/sage-devel
URL: http://www.sagemath.org
