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 _______________________________________________ et-mgmt-tools mailing list et-mgmt-tools@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/et-mgmt-tools
