[et-mgmt-tools] [PATCH] Port utility functions to Solaris
levon at movementarian.org
Tue Dec 9 22:25:57 UTC 2008
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).
> 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.
> > +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'.
> 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. Locking away the contents of util.py and never changing it
again makes it really hard to *NOT* maintain the API - any patch touches
that file implies the patch is broken.
Even maintainers as illustrious as your good self need help in spotting
 yes, I'd like to do this will ALL the legacy API, but I admit
that's most likely a bridge too far
More information about the et-mgmt-tools