On 03/10/10 02:56 AM, Frank Che wrote:
> The project team has updated the exported interface table according to
> the review feedbacks. I have just updated file 'spec.txt' in the case
> directory to reflect this change.
I'm sorry to keep harping on this point, but the python module and/or
package names are still not included as an exported interface, and that
seems like an important detail. Programs need to "import <module>" to
get at this stuff, right? What is that? Assuming that we resolve this
issue, I give the case a +1.
That said, looking more closely at the API, I have some additional
feedback. It's a Consolidation Private API and these are all
superficial issues, so I don't feel strongly about the below issues.
* There's no need to prefix all of your symbols with "ddu_". Don't you
need to access your base methods using <module>.<function>() anyway?
E.g., ddu.devscan() seems cleaner than ddu.ddu_devscan().
* The class names all have an "_object" suffix, which is strange.
Here's are suggested changes:
- ddu_dev_data_object -> device
- ddu_repo_object -> repository
- ddu_package_object -> package
* Your accessor functions (get_location(), get_name(), and get_type())
are unnecessary, and should be attribute references (e.g.,
package.location, package.name, and package.type).
* The ddu_install_package() function could easily be a method of the
package class. Instead of passing a package object to the
ddu_install_package() function, it would be more logical to simply call
an install method of an instantiated package object.
* I have a similar comment regarding the ddu_package_lookup() function
taking a device object as an argument. You're looking up the package
object that is associated with a device, so the package lookup function
should really simply be a method of the package object.
To put this together, you currently essentially have (distilled from you
code snipet example):
device_list[] = ddu_devscan(return_missing_only, "all")
repos[] = ddu_build_repo_list(list of (name, URL) tuples)
for device in device_list:
# Look up the package for the given device.
ddu_package_obj =
ddu_package_lookup(device, repos[])
ddu_install_package(ddu_package_obj, "/")
...
This would become something like:
devices[] = ddu.device_scan(return_missing_only, "all")
repos[] = ddu.build_repo_list(list of (name, URL) tuples)
for device in devices:
# Look up the package for the given device.
ddupkg = device.getpackage(repos[])
ddupkg.install("/")
-Seb
