Hi Sebastien.
On 03/10/10 07:50 AM, Sebastien Roy wrote:
> 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.
Since I wrote the specs that the below issues deal with (with the help
of the DDU team, of course), I feel it is appropriate for me to respond...
For convenience regarding modifying the case documents, current
filenames and the classes/functions they contain are:
ddu_devdata.py :
ddu_dev_data class
ddu_errors.py:
contains all DDU-specific exception classes.
ddu_function.py:
ddu_build_repo_list()
ddu_devscan()
ddu_package_lookup()
ddu_install_package()
ddu_package.py:
ddu_package_object class
ddu_repo.py:
ddu_repo_object class
> 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().
Yes and no. One can access methods as you say, but more commonly, one
says at the top of a python module:
from DDU.ddu_function.py import ddu_package_lookup
and then uses ddu_package_lookup henceforth in that module without a
prefix. No need to muddy up the code unnecessarily with <module> every
time you call a function.
This is standard practice and what the DDU code does.
I think the function names can stay as they are.
>
> * 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
I'd like to leave this for now.
>
> * 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).
OK. This needs to be changed in the spec, but the code is already like
this.
>
> * 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.
This is a good suggestion and I'd like to follow up on it at a later date.
>
> * 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("/")
This is another good suggestion that I'd like to follow up on it at a
later date.
I plan on filing a bug for these changes so they don't get dropped on
the floor, after I take the DDU team's input (tonight).
thanks,
Jack
>
> -Seb
