On 08/10/10 10:52, Keith Mitchell wrote:
Hi Ethan,
Responses inline.
On 08/10/10 07:51 AM, Ethan Quach wrote:
Keith,
Thanks for the comments. Replying to:
AI_database.py
publish_manifest.py
set_criteria.py
On 08/09/10 12:11, Keith Mitchell wrote:
Hi Ethan,
I focused on the Python files, but scanned the others as well.
Please forgive any duplicated comments; I haven't had time to read
through the other reviewers' responses - just point me to your
response to them as needed.
- Keith
AI_database.py:
298-300, 333-334: This seems like a rather narrow, single-purposed
addition to the function - perhaps it could be excludeManifests and
accept an iterable / list?
Sure I could make that change, but I don't see that huge
of a benefit of doing that now than changing it to be so
if/when we'd ever need it be a list.
That's fine. The only potential issue is that it's more difficult to
change the 'type' of an accepted parameter later, as you have to start
updating the callers.
I've made this change.
[...]
126: Pre-existing, but this error message doesn't tell the user how
to fix it. Should they delete the service? Manually add the needed
SMF prop? (Sorry, my eye wanders during reviews).
If the SMF property group lacks either of those properties,
there isn't an exact prescription for a fix. I don't think
deleting the service would the the best thing to tell users
to do either. We could perhaps include the name of the
property that failed to exist to be a little bit more informative.
If that's possible to update, that'd help debugging if it ever came up
(though I imagine it's highly unlikely in any case).
I'm printing out the exception argument now, and it is the key
which caused the keyerror, so the it will show up in the output.
[...]
157: As I recall from some conversations with Sue, isn't one of the
values for criteria a filename? If so, isn't the lower() call going
to have an adverse affect for files that have capitals? I can
understand calling "lower()" on all the keys at this stage, but
lowercasing the values at this stage seems unnecessary.
No, values for criteria aren't filenames. Perhaps you're thinking
about the -C option, which is a filename.
None of the criteria *now* are case-sensitive filenames - but can you
guarantee that none of the values in the future will ever be case
sensitive (files or otherwise)?
This will have to be addressed uniformly throughout then.
Many places in the code do a lower() and sometimes upper()
of the values as they're parsed in and spit out from the DB
storage. If case-sensitivity is indeed a problem, it needs to
be addressed properly.
I will consult with Clay on this issue and file a bug as
necessary, but for now I'd like to leave that line alone.
thanks,
-ethan
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
