Hi Jack,
Standard disclaimer: Suggestions which are out-of-scope / detract from
the goal of completing the 2.4->2.6 conversion can be dealt with later.
Thanks,
Keith
General: Module names should be lowercase with underscores, though this
kind of refactoring is deferrable.
install-tools/ManifestRead.py:
144: "is not None:"
install-tools/ManifestServ.py:
76: Did you intend to put "if not path:" here?
As a more general comment, this loop would be more appropriate if the
while clause evaluated path in some way, e.g.,
path = sys.stdin.readline()
while path:
...
<do_stuff_here>
...
path = sys.stdin.readline()
78-79: As the main clause catches KeyboardInterrupt, why bother catching
anything here?
237: "is not None"
239: Extra space between exit and the open paren.
DefValProc.py:
33-38: The original import statements were more readable, I think.
39: This import should be above our osol_install imports (with a blank
line between)
57: I think this should point to python2.6 now
108: Nit: This function doesn't return anything.
170-176: Can remove this and replace with: "if ref not in methods:"
182-183: "if module_name.endswith(".py"):" (Unless we have cases where
we are evaluating names that are, e.g.,
"modu_name.py_some_extra_suffix"). Line 188 would need to be modified
accordingly.
197-204: I don't see anything here that would warrant the try/except block.
208: "if not match:", or combine this block with the previous for loop
using for/else syntax.
Based on 187 and 211-227, this code looks very fragile. At a minimum,
the constraint of module name = class name will force us to break PEP8
naming conventions. Can you file a bug for looking into alternatives to
this usage scenario (assuming that the 2.4->2.6 name changes here and
elsewhere don't cause this code to break, forcing you to come up with a
solution now)?
424-430: Could be "if method_ref not in deflt_setter_dicts.modules or
not in deflt.setter_dicts.methods:"
443-446: By wrapping the exceptions as such, we're masking valuable
stack trace information.
490-494: The 'get' method of dictionaries allows you to streamline this
to a single line as "skip_if_no_exist =
attributes.get("skip_if_no_exist", "")"
610-611: Could be "parent_nodepath, child_nodepath =
manifest_nodepath.rsplit("/", 1)"
639-644: Again, this could be simplified to "no_parent_handling =
attributes.get("missing_parent", "error")"
683-688: Same thing.
717: Change this to "!=" instead of "not ... ==" would be easier to read.
718, 816: Using single quotes here means you don't have to escape the
double quotes.
845-856: Again, the dictionary.get method would be appropriate here.
Also see comments for 424-430
857-858: Should just check if validator_dicts.inverts is not None first...
953-957: This change masks potentially useful traceback data.
970-1002: Instead of a dummy variable and catching KeyError, using "if
<key> in <dictionary>:" syntax would be more appropriate.
1060-1061: See comment on 610-611
1068-1072: Again, dictionary.get(...) would be ok.
1126: "if errors:" (Sidenote: If we raise an exception if any error
occurs, why don't we raise right away, to provide better context for
where the error occurred...)
1127,1188: Change to "raise ManifestProcError(..."
1187,1259: "if errors:"
1265, 1316: Sidenote: These functions will mask underlying errors and
may make things more difficult to debug.
ENParser.py:
Side comment: __STATE_TABLE might be better off as a dictionary, but
that's certainly out of scope ;)
406: Should be "if curr_char in __SPECIAL_CHARS:"
482-484: Could be replaced with "for curr_char in nodepath:"
install_utils/ManifestRead.py:
General: In "except <SomeError>, err:" clauses, if err is never used,
please change to just "except <SomeError>:"
37: Please group this import with errno and sys
146-147: Style note: In this scenario, the '+' is optional.
178: Re-align
194-202: Change to "except (socket.error, IndexError, ValueError):" or
"except StandardError:" Actually, this is one of the scenarios where a
bare 'except:' clause is acceptable, as you simply log that an error
occurred and re-raise.
219-220: Splitting the line as such makes this difficult to read. Use
two lines as such:
results = self.client_sock.recv(size)
results = results.split(SocketServProtocol.STRING_SEP)
install_utils/ManifestServ.py:
55: Please base this class off of StandardError.
171-172: This might be more readable as:
manifest_path = full_manifest_basename.rsplit("/")
manifest_basename = manifest_path[-1]
188, 243: 'err' is unused here
209-215,226-232: Please leave this as part of the except clauses. Use
the syntax "except (KeyError, ManifestProcError), err:" to catch
different error classes in a single clause.
273-276: Please combine these except clauses.
304-307: I think the previous syntax of re-raising the thread.error was
more appropriate. Masking it as a ManifestServError loses some of the
traceback information.
340-343: I think this should be in a 'finally' clause.
448: I think this should be "if not pre_request:" or left unchanged.
Also, this 'if' clause should probably just be the subject of the
'while' clause.
495: "if not values:"
516,522: Similar comment
553: Here, and in the other location(s) where you make this note, I
think it's appropriate to add the in-line comment that disables it, and
reference http://www.logilab.org/ticket/8764 as the reason for disabling
temporarily.
590-604: Why are we now returning instead of re-raising?
TreeAcc.py:
37-39: Please move these imports above the osol_install imports.
190-191: Could be combined to one line.
259-260: Ditto
414-415: Will this fit on one line now?
571-575: This change causes us to lose some traceback information.
812: This final return statement is unnecessary. Sidenote: This function
would be more appropriate if it returned found_nodes, and the callers
were updated to set found_nodes = __search_node(...)
1030-1032: Check alignment on these.
1369-1372: Seems like this should be a finally block after 1361-1366.
232-235: Just don't catch theses, and modify 236 to "except
StandardError:" as the 'catch all'
254-258,289-293: Ditto
306: This isn't in scope at all, but changing epspec to be a dictionary
instead of a list would make it less fragile.
395: "if not module:"
399: This seems to be a pretty significant change that removes the
ability to specify "python-module:function" as indicated by the function
docstring. If this is intended, please update the docstring; if not,
please revert.
436-437, 439-440: Sidenote: If arg is always a string to begin with, we
could convert these to "shell_list.extend(<list>)"
install_utils.py:
255-265: Could be simplified to:
try:
int(arg, 0)
float(arg)
return True
except ValueError:
return False
366: At minimum, let's set this to "while True:" (though actually
evaluating a condition would be even better)
382-386, 412-415: Could be replaced with "stdout_buf =
stdout_buf.rstrip('\n')" (
424: "if output.endswith('\n'):"
426-427: Line 426 is not needed, just pass stderr_buff[:-1] to log.log(...)
Jack Schwartz wrote:
> Hi everyone.
>
> Here are 2.4 -> 2.6 and PEP8 changes for install_utils used by DC and
> other parts of the install mosaic. Please review. (Clay, are you
> available for this?)
>
> http://cr.opensolaris.org/~schwartz/091101.1/webrev/index.html
>
> All files achieve a 10 by pylint, with the following caveats:
> - usr/src/lib/install_utils/ManifestServ.py: --disable-msg=C0321 which
> thinks the try of try/except/finally is two statements on one line
> - TreeAcc.py: --disable-msg=E1103 since pylint thinks some datatypes
> are lists when they are not.
> These are documented in the code.
>
> I have tested all changed files:
> - tested parser files by executing all functions of TreeAcc module
> - tested many different nodepaths to exercise ENParser module
> - testing both good and error cases for validation and default setting
> - verified ManifestRead socket interfaces
> - verified finalizer called modules with args correctly
>
> There are many changed lines, but most are formatting for PEP8, so it
> shouldn't take too long...
>
> Changes other than formatting changes include:
> - condensing some nested (try/except - try/finally) into
> try/except/finally
> - changed reader in TreeAcc from xml.dom.ext.reader to xml.dom/minicom
> - wrote a tree walker in TreeAcc to replace the one which went away in
> 2.6
> - got rid of Trace in install_utils
> - removed py function call handling (which never worked and gave 2.6
> pylint errors) from finalizer
>
> Thanks,
> Jack
>
>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
