Keith, I'll let Evan and Darren (when he returns next week) add their comments on the remaining issues below (49-55; 57; 110-118) as they were involved in designing and writing some of this code and may have stronger opinions on it.
Re: > Just to clarify - I'm suggesting removing these functions entirely (as > opposed to having the functions call isinstance). That's what I understood. Thanks again, - Dermot On 01/05/10 22:01, Keith Mitchell wrote: > 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
