Keith, I think you and I have slightly different opinions about how widespread the use of exceptions should be ;)
Anyway, this one is not a big deal and seeing as Evan has already made your suggested change, I'll agree with you on this one... - Dermot On 01/06/10 15:54, Keith Mitchell wrote: > Hi Dermot, > > Dermot McCluskey wrote: >> 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? > > I still disagree. It would be good to have a unified, automated test > framework for running unit tests down the line. The portion of said > framework for testing Python would need to be able to continue running > even if a single module's unit tests failed. And since it is meant for > developers, not end users, I don't see a problem with an ugly traceback > (especially on an import failure, which would indicate a very strange > situation in which the developer would likely want the error message > from the ImportError). > > By the way, I think the unit test module with your code is great! It > gives us a starting point for building such a test framework. > > - Keith >> >> >> - 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
