Hi Jack,
On 03/10/10 03:52 PM, Jack Schwartz wrote:
> 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.
It's your call, but honestly, your argument doesn't seem logical
(regarding muddying up the code) given the difference is between
ddu_package_lookup() and ddu.package_lookup()... The difference is a
single character. It seems like you're reinventing a namespace
convention that's already provided for you by the python module
framework. Anyway, it's up to you and your code-reviewers, I don't feel
strongly about it.
>
> 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).
That's all fine by me.
Thanks,
-Seb
