On Tue, Jan 24, 2012 at 08:47:21AM +0800, Paul Wise wrote:
> I always get curious when people upload with no comments, so here is a review:
First of all, thank you for the review. I’m always looking forward to
improve my packages, and an in–depth review gives me the chance to do
just that.
> You forgot to mention collab-maint in the changelog.
Yeah, I should probably have mentioned it, but mostly due to the fact
that I’ve added Vcs-* fields — or is collab-maint considered special
in this regard?
Do you suggest mentioning the addition in the changelog entry for the
next upload, or editing the current entry? The latter seems hacky, but
the former is a plain lie.
> You didn't mention the reason for changing the priority to extra.
rhinote depends on cups-bsd, which has Priority: extra, and according
to Policy packages can’t depend on packages with lower priority.
Since I’m going to have to patch the source anyway due to the suggestions
you made below, I might try to make it use lp where available (with
fallback to lpr), and depend on cups-client | lpr (both of which are
Priority: optional) instead.
I believe I could then drop the dependency on cups-bsd | lprng, since
both packages provide lpr, and raise the priority to optional again. Is
that correct?
> Please forward the desktop file and manual page upstream.
I’ve already forwarded them; upstream will probably include them in the
next upstream release, whenever that might be. Any suggestions on how to
document this fact? If we were talking about a patch, I would just use
DEP3, but this is a different scenario…
> One warning from pyflakes to forward upstream:
>
> rhinote.py:24: 'from Tkinter import *' used; unable to detect undefined names
I will notify upstream about this, maybe after doing some research to
understand what it means ;)
> rhinote.py should use os.path.expanduser and instead of
> os.environ['HOME'] since that works when the HOME environment variable
> is not set. Please send upstream a patch for this.
Unfortunately I’m not aware of the various Python best practices due to
the fact that I only did little Python programming; thank you for pointing
this out, I’ll patch it and notify upstream.
> rhinote.py should use the python subprocess module instead of the
> system() function. In any case this thing will not work since it will
> write to a file named lpr instead of running the lpr program. With
> subprocess you can easily pipe the results of one program to another
> which is what rhinote appears to want to do here. Also for upstream
> (but not on Debian due to the Depends), lpr/enscript are not
> guaranteed to be installed so this will fail silently instead of
> informing the user that printing is not available. In addition it
> doesn't remove the ~/.Rhinoteprintfile file when it is complete,
> leaving files around in ~/ is rude. I would also suggest that rhinote
> should be using a temporary file in the temp dir instead of a file in
> ~/ for this purpose, please ensure that it is secure and also supports
> $TMPDIR by using the python tempfile module.
>
> system('enscript -B --word-wrap $HOME/.Rhinoteprintfile > lpr &')
>
> Please send upstream a patch for this to use the subprocess module,
> detect when enscript/lpr are available and notify the user if not, use
> the tempfile module (for security and to support $TMPDIR etc) and also
> clean up afterwards.
I have to admit I’m guilty of never having attempted to print using
Rhinote, mostly due to the fact that I rarely have a printer handy.
Lesson learned.
I will try to conjure a patch addressing all the concerns you raised
during the course of the next few days, and I hope you’ll be willing to
review it when it’s done to make sure it is suitable for inclusion
upstream. As I’ve mentioned earlier, I don’t have deep knowledge of
Python, but you sure seem to know your way around it.
Once again, thank you for your review.
--
Andrea Bolognani <eof@kiyuko.org>
Resistance is futile, you will be garbage collected.
Attachment:
signature.asc
Description: Digital signature