[et-mgmt-tools] [PATCH] Port utility functions to Solaris

[Cole Robinson]

John Levon wrote:
> On Tue, Dec 09, 2008 at 05:13:05PM -0500, Cole Robinson wrote:
>
>   
>>> Port utility functions to Solaris
>>>       
>> Hmm, 90% of this patch is a huge mechanical change which has nothing
>> to do with this commit subject. I think it's better to separate
>> the two changes.
>>     
>
> Seems like busy work to me quite frankly. You can diff util.py versus
> _util.py to get the changes easily enough (I tried a rename, but hg was
> playing silly buggers).
>
>   

I really don't think it's unreasonable to ask that these two unrelated
changes be split. The commits will be more self contained, and it will
be easier for review.

>> Rather than s/util/_util/ everywhere, I'd prefer:
>>
>> import _util as util
>>
>> Lot less churn, and the only potential downside is confusion
>> browsing the source, but I don't think it's a problem.
>>     
>
> It's already confusing enough, please let's not make it worse.
>
>   

I won't disagree with that.

>>> +lookup_pool_by_path = _util.lookup_pool_by_path
>>> +privileged_user = _util.privileged_user
>>>       
>> Hmm, this isn't exactly how I saw it happening. I figured we would have
>> _util be for nonpublic functions only: we can change or remove anything
>> in that file at will. The original 'util' though needs to maintain api
>> compat for its functions, even if they are deprecated and left to rot.
>>     
>
> The public API has been clearly moving towards an OO approach. This
> makes perfect sense to me: if I want to interact with a disk, use
> VirtualDisk. Anything new in virtinst.util is a bug - can you name a
> situation where it's sensible for something to be added there as part of
> the API? The very name 'util' basically 'random stuff'.
>
>   

Yes, that's fair. Thinking of it that way, there will always be a better
solution than dumping something in 'util'.

>> Am I missing some benefit of the above approach?
>>     
>
> APIs are really hard to maintain when they're commingled amongst a bunch
> of other completely unstable code, especially when it's not even
> documented.

I completely agree with this, that's why my suggestion was to keep the
public and private functions in separate files. Moving everything
to the private file seems like it would only encourage confusion
between what is public and what is private, increasing the likelyhood
that someone would inadvertently break API.

>  Locking away the contents of util.py and never changing it
> again[1] makes it really hard to *NOT* maintain the API - any patch touches
> that file implies the patch is broken.
>   

I don't follow this. By moving the actual content to _util.py, API can
_still_ be broken: someone could add a new function argument without
a default value.

I think we can both agree that one of the biggest failures of this
code base is a lack of documentation. It can be a total frustration
trying to cleanup and fix things, not knowing how the pieces interact.
I've been trying to remedy this as I go. Dealing with 'util' is an
important step, and I'm not trying to push back on it.

Thanks,
Cole