Re: RFS: rhinote (new upstream version)
On Tue, Jan 24, 2012 at 5:12 AM, Andrea Bolognani wrote:
> On Mon, Jan 23, 2012 at 08:54:57PM +0100, David Paleino wrote:
>
>> Uploaded, thanks for your contribution to Debian!
>
> Whoa, looks like I was right: it really was a quick job for a
> Debian Member ;)
>
> Thank you for the upload!
I always get curious when people upload with no comments, so here is a review:
You forgot to mention collab-maint in the changelog.
You didn't mention the reason for changing the priority to extra.
Please forward the desktop file and manual page upstream.
One warning from pyflakes to forward upstream:
rhinote.py:24: 'from Tkinter import *' used; unable to detect undefined names
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.
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.
--
bye,
pabs
http://wiki.debian.org/PaulWise
Reply to: