[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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[1] 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
mistakes.

regards
john

[1] yes, I'd like to do this will ALL the legacy API, but I admit
that's most likely a bridge too far


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]