On 02/15/11 08:24 AM, John Fischer wrote:
Hey Keith,
This looks really good. Just a couple of comments:
usr/src/lib/install_common/__init__.py
--------------------------------------
Only a nit please check your blank lines for spaces.
Much of the code I putback has spaces on otherwise blank lines. I have
two reasons for this:
* When using editors that show whitespace, it looks cleaner, as the code
will align
* When copying and pasting code directly into the python interpreter
(which I'll do sometimes to verify it), the spaces are required to
properly parse.
Note that the pep8 tool will warn about this, but has the following to
say about it:
__init__.py:328:1: W293 blank line contains whitespace
^
JCR: Trailing whitespace is superfluous.
FBM: Except when it occurs as part of a blank line (i.e. the line is
nothing but whitespace). According to Python docs[1] a line
with only
whitespace is considered a blank line, and is to be ignored.
However,
matching a blank line to its indentation level avoids mistakenly
terminating a multi-line statement (e.g. class declaration) when
pasting code into the standard Python interpreter.
[1]
http://docs.python.org/reference/lexical_analysis.html#blank-lines
Since the official word seems somewhat ambiguous, and the whitespace has
a (small) benefit, I'm inclined to leave it in.
usr/src/lib/install_common/test/test_install_popen.py
-----------------------------------------------------
It seems that the comment should appear before the assertFalse() or
that there is something else missing.
128 self.assertFalse(100 in logger.msgs, logger.msgs.get(100,
''))
129 # No output logged to the MockLogger
I'll reverse that. I occasionally have a strange tendency to put a
comment after the line, instead of before it. I'm not sure where I
picked up that habit...
Anther nit is that the file is not pep8 clean, blank lines with spaces,
operators without spaces around them, blank line at end of file, ...
I'll address the outstanding pep8 issues.
Thanks,
Keith
Thanks,
John
On 02/14/11 05:50 PM, Keith Mitchell wrote:
Hi all,
Can I get a review of my changes for these bugs please?
Bugs:
7019067 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7019067>
solaris_install.Popen logs blank lines
7018063 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7018063>
solaris_install.Popen should allow a user to ignore the check_code check
7018060 <http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=7018060>
solaris_install.Popen should offer a DEVNULL destination
Webrev:
http://cr.opensolaris.org/~kemitche/webrev.7019067
In addition to resolving the listed bugs, I've:
- Added usage examples to the docstring for Popen
- Added docstrings to the test cases
- Resolved a bug where subprocesses that completed quickly may
hang in the select.select() call when attempting to perform logging.
(e.g., running Popen(["pkg", "foo"], logger=mylogger) would hit this)
Thanks,
Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
