-------- Original Message -------- Subject: Re: DDU code review logistics Date: Wed, 24 Feb 2010 12:58:27 -0800 From: Keith Mitchell <keith.mitch...@sun.com> To: Jack Schwartz <Jack.A.Schwartz at Sun.COM>
Hi Jack, Please forward these comments on to the DDU team. I was not able to look at other sections, and hit less detail than I might normally go into, but for the most part the code looks workable. - Keith Section: http://cr.opensolaris.org/~billyan/DDU_1.3.1/webrev/ddu_1.3.1_text_webrev/ General: Import statements: It would be better if absolute import statements were used. That is, use "import ddu-text.utils.action" instead of "import action", for example. ddu-text: ESCDELAY should not be blindly exported to 0 here, as it will cause ncurses to misinterpret keystrokes when logged in remotely (e.g. via tipline). Let utils/__init__.py set ESCDELAY (although it should be updated to default to 250 instead of 0). If that's done, then this file becomes unnecessary (deliver ddu-text.py as ddu-text, and add a python2.6 shebang at the top of it). ddu-text.py: Logging statements in this file and others should be updated. There are references to the 'text installer', and the log entries are sent to /tmp/install_log. Line 38: Should be removed 86: Remove this line (or uncomment it, if ctrl-C should be ignored) device_scan.py: 71-73: You may want to remove/update the code in main_window.py that adds these actions by default, and supply your own (this comment also applies to media_scan.py) 120: The python builtin "isinstance(...)" function would be more appropriate here, e.g.: if isinstance(self.main_win.central_area.objects[0], InputWindow): 150: The 'return' statement here isn't needed 201-203: This would achieve the same result without a for loop: space_chr = '-' * (win_size_x - 10) media_scan.py: 183-186: This 'except' clause doesn't look like it does anything -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20100224/d4dfe2be/attachment.html>
