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 <firstname.lastname@example.org> Resistance is futile, you will be garbage collected.
Description: Digital signature