Hi Keith, Never mind and please continue to ignore me on this! ;-)
I got confused a bit on what you where referring to on some of this but what I'd like to do on these items is wait until Darren is back from vacation and get all of us together with you on Monday and go over this to hash out what we really want to do here. Thanks! -evan On 1/6/10 8:46 AM, 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. > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
