Hi Dermot, Dermot McCluskey wrote: > Keith, > > This is responding to all your remaining comments, except > unittesting.py, which I will leave Luis or Evan to discuss. > > >> 49-55: Is there any reason to not use direct attribute access? > > I think it's fairly standard to provide setter and getter > methods in cases like this. Also, it means the C and Python > user code is similar because there's a function to call > in both cases.
The general case for Python is to start with direct attribute access, and transition the attribute to a "property" at a later time if getters/setters become necessary. At a minimum, you should start with the property syntax, so that there is only a single interface to mod_id (right now, consumers could use either self.mod_id or self.get_mod_id()). In the case of the C code, there are API functions for retrieving an attribute that should work. However, if they don't (or it's not easy to make them work in this case for some reason), using the property syntax would still be preferred for Python consumers. > > >> 57: Why the use of return codes in Python code? > > This function raises an exception if an invalid "type" > param was passed in (ie a programming error), but returns 0 > if the value doesn't match the type (a possible runtime > scenario). That seems about right to me. > > And again, it means that the C and Python user code > will be more similar. I would like to avoid "C-isms" in new Python code (and I'm pushing to get rid of them in existing code as well). You can differentiate the two error cases by raising different exceptions, or by passing args to the exception that are parsed. If the only difference is the error message shown, the easiest and most appropriate way would be to raise a TypeError with the desired message, and have consumers (whether C or Python) display the message. If it's more than a message issue, then adding a second arg for differentiating, or using a different exception class (possibly a custom sub-class of TypeError) would be more appropriate. > > >> >> 65: Why this syntax? If this list is expected to grow later, can we >> define it as a constant of the class? > > ok - will do. > > >> 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. > > Right - this function is only used for testing, so it's > not a performance issue. Yup - just bringing up awareness (and, ideally, convincing you to change it so that consistency is kept). > > >> 110-118: Could this be incorporated into the ErrorInfo.__init__? Or, >> is it a valid scenario to create an ErrorInfo independently from the >> list? > > I think it's neater to have a function to call here. And again, > it makes the C implementation easier by having a function to call. My concern here is that we have two seemingly identical paths for a Python consumer to create an error info that act in different ways. My personal preference is also to have code patterns match paradigms (e.g., a Python consumer should be able to instantiate a class in the 'normal' way - in this case "my_err = ErrorInfo(...)") If having a 'helper' function to instantiate is easier for C consumers, then the function can stay, but please move line 117 into the __init__ code. (And if we need to be able to create ErrorInfos without adding them to the __errors list, than a True/False keyword argument to the __init__ function could default to adding to 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. > > > It could be done that way, but I don't see anything wrong > with the current implementation. Bear in mind that in practice > there will rarely be more than one or two errors in existence > at any time. So, I don't think re-writing for maximum > efficiency is necessary. Fair enough. > > >> 157: The second check (errs > 0) is redundant - "if errs:" only >> returns True if a list is non-empty. > > good catch - will fix > >> 158: Nit: "for err in errs:" > > yes - will fix > >> 159: Nit: "print err" is sufficient. (And, using str(err) instead of >> err.__str__() is generally more common) > > ok - will fix > >> 178: Nit: integer -> string >> >> 163-185: Can you use the Python built-in isinstance instead? > > good suggestion - will change. Just to clarify - I'm suggesting removing these functions entirely (as opposed to having the functions call isinstance). > > > >> 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 > > thanks - will fix > >> >> liberrsvc_defs.h: >> >> 38-40, 41-42: Nit: Make these into single block comments so they're >> more readable. > > ok - will fix > > >> 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? > > see previous email from Evan. > > > > Thanks again for reviewing. No problem! - Keith > > > - Dermot > > > > >> >> 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
