Keith, Just addressing one point I skipped previously:
> unittesting.py: > ... > 41: Nit: Instead of sys.exit(1), re-raise the exception (or raise a new > one). Calling sys.exit(1) prevents, for example, writing a meta-module > that runs a series of unittests (if the import of this module failed, > and a reasonable exception is raised, the managing module/script can > catch it and move on to the next set of tests - if sys.exit(1) is > called, the rest of the script wouldn't get run). unittesting.py is an executable script and is not intended to be imported elsewhere. It's a quick way for developers to check 'did my recent changes break anything?'. There's a separate set of TET test cases being written elsewhere. So, given all that, I think it's much neater for the script to exit gracefully with an error message rather than throw out an ugly traceback, in this instance. Do you agree? - Dermot On 01/04/10 15:12, Keith Mitchell wrote: > Hi Evan (and the rest of the Error Service project team), > > I took a good look at errsvc.py, and a briefer look at the C files > (except for the test_*.c files). My comments are below. Looks pretty good! > > - Keith > > errsvc.py: > > General nit: There are some extra spaces around parens in this file > (e.g. at lines 38, 57, 110, etc.) > > 40-42: Can we define this list as a constant somewhere? > > 49-55: Is there any reason to not use direct attribute access? > > 57: Why the use of return codes in Python code? > > 65: Why this syntax? If this list is expected to grow later, can we > define it as a constant of the class? > > 88: Performance is likely not an issue here, but in general, string > concatenation is much more efficient if you create a list of strings, > and then use "\n".join(<list>) or something similar. > > 110-118: Could this be incorporated into the ErrorInfo.__init__? Or, is > it a valid scenario to create an ErrorInfo independently from the list? > > 133: Since we control the adding of errors to the list, would it be > easier to simply store separate lists for each type? Or have the global > __errors be a dictionary, where the key is error_type and the value is a > list of errors of that type? It seems that conglomerating lists together > when the full list of all errors is needed would be simpler than picking > apart the list to get errors of a certain type. > > 143: Same question. Since we control adding items to __errors via the > create_error_info method, it seems like managing multiple lists (or > dictionaries, as appropriate) would be relatively simple. > > 157: The second check (errs > 0) is redundant - "if errs:" only returns > True if a list is non-empty. > 158: Nit: "for err in errs:" > 159: Nit: "print err" is sufficient. (And, using str(err) instead of > err.__str__() is generally more common) > > 178: Nit: integer -> string > > 163-185: Can you use the Python built-in isinstance instead? > > unittesting.py: > > General: The error cases should be tested too (e.g., calling > set_error_data with an invalid type, or with a value that mismatches the > type, or instantiating an ErrorInfo with an invalid type). > > 31-32: Nit: platform.processor() is simple enough to make a call to, to > determine sparc or i386. > 41: Nit: Instead of sys.exit(1), re-raise the exception (or raise a new > one). Calling sys.exit(1) prevents, for example, writing a meta-module > that runs a series of unittests (if the import of this module failed, > and a reasonable exception is raised, the managing module/script can > catch it and move on to the next set of tests - if sys.exit(1) is > called, the rest of the script wouldn't get run). > > 49: Nit: Call it "self.error_types", since it's a list with more than > one type. > > 52: Why store this value? It's easily retrievable with a call to len() > > 57, 67, 72: Use "for error_type in self.error_types" instead of creating > the arbitrary index variable c. It's rare to need to know the index of > an item in a list in Python (and when you do, use of the enumerate() > function is the best way to track it when looping - line 116 is a good > example: "for idx, item in enumerate(self.error_types):") > > 66, 96, 115: This should be in a tearDown method. > > 140: Should this be a liberrsvc constant? > > errsvc.c: > > General question: What's the advantage/purpose of the use of threading > in this code? > > General note: There are a few comments that are just "Call Python" - it > would help if the comment were expanded to indicate what Python code is > called (especially since the Python method called can get obscured by > the fact that it takes quite a few lines of C code to get to the point > of being able to call said method) > > 61: What's the purpose of hardcoding the path here? Seems like this > would cause maintenance problems and reduce flexibility. > > 110: "UNKNOWN ERROR" - should this be #defined? > > 566-571: I don't quite understand the comment. It seems like calling the > clear_error_list function should take care of clearing out the error > list. Where are the extra references coming from that need to be > decremented here, instead of by whichever portion of the code grabbed > hold of the extra reference in the first place? > > 683: As mentioned above, Python code should raise errors, and not return > error codes. Reserve return values for useful data, and check for > exceptions if attempting to handle an error condition. > > 1030-1078: This bit of code looks very similar to blocks of code in > other functions. Is it possible to combine somehow for the purpose of > simplifying, or are they different enough that it's not possible to do > such a thing in C? > > liberrsvc.h:60: Nit: _LIBERREVC_H -> _LIBERRSVC_H > > liberrsvc_defs.h: > > 38-40, 41-42: Nit: Make these into single block comments so they're more > readable. > > prototype_com: > > General: The osol_install directory seems to be getting a little > cluttered, perhaps we need a 'utils' directory for things like the error > service, the future logging service, and so on? > > Evan Layton wrote: >> The CUD Error Service project team is requesting a code review of >> the project gate. >> >> The code implementation is complete and unit testing has been done. >> The official testing is in progress but is not yet complete. We will >> be providing updated incremental webrevs as code review comments and >> bug fixes come in. >> >> The webrev is available at: >> http://cr.opensolaris.org/~evanl/CUD_errsvc >> >> With the holiday coming up it appears that most people will be out >> of town for a while so we'll be giving extra time for folks to look >> at this. We'd like to have all comments back by January 15, 2010. >> >> Thanks! >> >> -evan >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
