Evan, Clearing up one point:
The item below is a slightly different matter. Keith is (politely) saying that the 2 functions is_valid_integer() and is_valid_string() that I wrote are useless and can be replaced by calls to the built-in functions isinstance(my_var, int), etc. >> Just to clarify - I'm suggesting removing these functions entirely (as >> opposed to having the functions call isinstance). > > I'm very much against removing the getter and setter functions. While it > is true that a python consumer can directly access these attributes or > properties that is not what we want them to be doing. Yes right now most > of these functions are very simple but that may change in the funture > and we will want to be able to make these changes with a minimun of > change to the interfaces used. If a consumer is not suing these and we > change the underlying functionality that consumer may break and have to > change what they're doing where they won't have to if they where using > the provided interfaces. Based on this we do not plan to remove these > functions. - Dermot On 01/06/10 15:46, Evan Layton wrote: > Hi Keith, > > Some thoughts for on the remaining issues below (49-55; 57; 110-118). > We'll still need to get Darren's input on this when he gets back... > > Thanks for the review and comments/ideas on improving things!!! > > -evan > > On 1/5/10 3:01 PM, Keith Mitchell wrote: >>> >>>> 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. > > While we can change these accessor functions to use the property syntax > having a consumer do that directly is definitely not what we want here. > I don't see a problem with making the changes we discussed to use the > property syntax but we want the flexibility and extensibility that the > accessor functions provide. > > >> >>> >>> >>>> 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. > > This is less a C-ism than a common experience but that doesn't mean we > can't change this so it doesn't return 0. I tend to agree with Dermot on > this that it gives us a better way to deal with things for C code that > has to call into this. Part of the reasons for ding this here also have > to do with the fact that this is embedded Python code that has to play > nice with the C code that calls it. We'll need to run through this with > Darren when he gets back to make sure we do the right thing here. > >>> >>> >>>> 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). > > The function call is what we want there not only for readability but > because once again this is embedded Python code that must be callable > form the C library. Also I'm not sure I understand what you mean by > having two paths for a Python consumer. While it's true they could do > things outside the interfaces provided there's no guarantee that the > functionality won't change and if the consumer isn't using the provided > interface they may not continue to function and will be on their own to > fix that... > > As for removing line 117 I'd rather not do that since this is a much > simpler and readable way to add the new error into the list of errors > what the error is created. > > > >> >> Just to clarify - I'm suggesting removing these functions entirely (as >> opposed to having the functions call isinstance). > > I'm very much against removing the getter and setter functions. While it > is true that a python consumer can directly access these attributes or > properties that is not what we want them to be doing. Yes right now most > of these functions are very simple but that may change in the funture > and we will want to be able to make these changes with a minimun of > change to the interfaces used. If a consumer is not suing these and we > change the underlying functionality that consumer may break and have to > change what they're doing where they won't have to if they where using > the provided interfaces. Based on this we do not plan to remove these > functions.
