Hi Keith, On 07/26/10 06:27 PM, Keith Mitchell wrote: > On 07/26/10 08:24 AM, Darren Kenny wrote: >> On 07/23/10 11:47 PM, Keith Mitchell wrote: >>>> So I suppose to achieve this I would have to put everything into >>>> __init__.py, >>>> I just am not a fan of the "one big file" for all sources - it's harder to >>>> maintain, and gets messy over time. >>>> >>>> How would people feel about doing this in general? >>>> >>>> >>> Yeah, I agree that "one big file" is not a terribly clean approach. >>> >>> Some options to consider: >>> >>> solaris_install.data_object # For DataObject >>> solaris_install.data_object.cache >>> solaris_install.data_object.dict >>> >>> Or, >>> >>> solaris_install.cache # DataObject AND DataObjectCache in __init__ >>> solaris_install.cache.dict >>> solaris_install.cache.some_other_subclass # Other subclasses of >>> DataObject get separate files >>> >>> I like the first one better, because the names read really well when >>> compared to the names of the classes that will be getting imported. Of >>> course, open to other options as well. >>> >>> >> I think I like the first one best too - most consistent with the class names- >> will give this a go... >> >> ... >> > > Sounds great.
Taken one step on this, but still need to figure out the logic for solaris_install - specifically for creating/merging into solaris_install/__init__.py... ... >>>>> 264: It might be worth it to wrap a call to get_children(name, >>>>> class_type) and then just return the 1st item from that list (if not, >>>>> simply return child - no need to have a "found_child" variable). >>>>> Additionally, it seems more likely that a call to get_first_child would >>>>> raise an ObjectNotFound exception than a call to get_children. Finally, >>>>> it could be argued that not providing a class or name parameter to this >>>>> function is not particularly valuable (depending on your thoughts on >>>>> whether ordering really matters for anything other than items with the >>>>> same name). If only name is pertinent for ordering, and the data layout >>>>> is reorganized to be a dictionary, then this code becomes nearly >>>>> trivial: 'return self._children[name][0]' >>>>> >>>>> >>>> A call to get_children is potentially more expensive to locate an object >>>> that >>>> matches criteria - which is why I didn't go that route. >>>> >>>> As mentioned before ordering is important, and not just by name. >>>> >>>> >>> You're correct, the get_children call is far more expensive. I still >>> wonder if there's a way to combine the logic - would it be worth the >>> effort to have an optional "count" keyword arg to get_children, that >>> returns the first "count" children? >>> >> That could work, maybe if I added something like: >> >> get_children(name=None, class_type=None, >> max_count=None) >> >> So if max_count is specified then stop after that number has been reached. >> >> I prefer the max prefix to explain more what it does. >> >> As for keeping get_first_child, I will keep this since it's logic is slightly >> different in that it's expected to return only 1 object (not a list). >> >> ... >> > > Sounds good to me. Ok, I've done this, and things are working as expected. > >>>>> 470-481: I'd recommend having separate functions for "insert one" and >>>>> "insert all items in an iterable". (See list.append vs. list.extend). It >>>>> provides a cleaner disjunction and allows for one to create and insert >>>>> "list like" DataObjects. >>>>> >>>>> >>>>> >>>> Do you mean that I should have two DataObject.insert_children methods - >>>> maybe >>>> insert_child and insert_children? >>>> >>>> Do you really think it's likely for there to be a DataObject sub-class that >>>> would also be a list (when I say list here, I mean would match >>>> isinstance(.., list) - not just allow [] type access). >>>> >>>> >>> Consider the reverse - what happens when I pass in a tuple of >>> DataObjects? Or any iterable that isn't strictly a subclass of list? >>> >> I this case it will throw an exception since we only allow DataObject or >> list() to be passed - a tuple isn't supported here. >> >> Do you think it would be better to support any iterable object? >> >> But I still don't see a requirement to have separate methods... >> > > I do think it would be better to support any iterable, to save us from > odd bugs and improve flexibility. There's certainly good arguments for > allowing tuples, and perhaps sets, and at that point it's likely easiest > to just take on iterables as whole. > > After thinking about it for a bit, I think a single method could be > workable, with one condition - the check for whether to consider > "new_children" as a "list-like" or a single DataObject should do an > isinstance(new_children, DataObject) comparison. While it's possible > that a DataObject subclass could be "list-like", I doubt any list > subclasses will be DataObjects - and if they are, the caller probably > wants to insert the "container," as well as the items in the container, > into the DOC. Agreed, and implemented in cud_dc gate. >>> With all that said, the more I think about it, the more I feel it could >>> go either way. I will add the caveat that the logic will need to be >>> re-worked if it's left as an error, as an ObjectNotFound error won't be >>> raised if there are no children (lines 531-533), which is inconsistent. >>> >> I still don't see this as an error since there is no criteria specified - I >> feel that it's inconsistent to say ObjectNotFound if you never specified >> anything to match. >> >> In the case of a "delete all" you really don't care if there ever was >> anything >> there to delete in the first place... >> > > That's not quite what I meant. The delete all case is fine - I'm > considering the case where: > > The DataObject has no children > and > The caller calls > data_object.delete_children(name=some_specific_name) > > In this case, the caller expects children to be there, and to be > deleted. But due to the logic at lines 531-533, the call will return > successfully, when actually, ObjectNotFound should have been raised, > since nothing was deleted. I FINALLY figured out what you were referring to here - missed that issue, and have fixed it now. Thanks for your patience ;) Thanks, Darren. _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
