On 07/26/10 08:24 AM, Darren Kenny wrote:
Hi Keith,
On 07/23/10 11:47 PM, Keith Mitchell wrote:
Hi Darren,
Responses inline. Will there be a differential and/or v2 webrev available?
Posted an updated webrev this morning - didn't get the time on Friday to do
it.
I've cut out comment/history to try make this easier to read ;)
Hi Darren,
Thanks for the updated webrev. I've also trimmed items I have no further
comments on.
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.
Ah you're right, didn't quite think that through all the way. Uniqueness
doesn't rule it out (again, the value of the dictionary key-value pair
could be a list of all objects stored with that key), but checkpoints
are keyed by unique names rather than listed under the same key.
Makes me wish for the OrderedDict data structure in Python2.7.
Didn't know about that one - must keep an eye on it :)
...
I've been eyeing it for a while, but it's *just* out of reach. There are
some 'recipes' out there for recreating one in earlier versions of
Python, but it hasn't been enough of a need for anything I've done just
yet to justify implementing.
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.
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.
...
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.
...
Tests:
I'm going to be a touch more lenient in terms of style/implementation
here. For reference, can you generate a code coverage report and include
it in your reply to this email? (Your tests seem to run fine via the
slim_test runner in the cud_dc gate, so you should be able to just run
that with a --with-cover option to get the results. Optionally, check
"slim_test -h" for info on how to generate an HTML coverage report, and
include *that* instead of the text version).
Summary coverage HTML attached :)
The coverage scores look great - though in the future, perhaps they
could just be added to the webrev before uploading. Especially since a
few of the files are missing.
Hmm, ok - would that be just a matter of copying in to the webrev directory?
Yes (and optionally, adding a link to the webrev/index.html file -
though one could just add the direct link to the review email)
Are there 'hooks' that could be plugged into webrev to do this type of thing?
Not that I know of, though that'd be nice if there were. I'd like a hook
that publishes an hg bundle file with my webrevs...
...
I may not get around to reviewing the remaining test_* files in detail;
but from everything I've seen so far (including the coverage reports), I
expect they'll be great. Thanks for setting a great example with the
unit tests!
No problem, I think you've provided some great feedback :)
Thanks - hope I wasn't *too* harsh!
- Keith
Thanks,
Darren.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
