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

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



john levon sun com wrote:
> # HG changeset patch
> # User john levon sun com
> # Date 1228851891 28800
> # Node ID 29d8886362e2993eaf26cf8d4e948b2de4b8d9ec
> # Parent  3a48e2d3594b3e7e1d24147f3325ab6024557504
> 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.

> Create _util.py for private details of the implementation, along with a
> shim for back compatibility.
> 
> Port the utilities to Solaris.
> 
> Signed-off-by: John Levon <john levon sun com>
> 
> diff --git a/virtinst/CloneManager.py b/virtinst/CloneManager.py
> --- a/virtinst/CloneManager.py
> +++ b/virtinst/CloneManager.py
> @@ -23,7 +23,7 @@ import libxml2
>  import libxml2
>  import logging
>  import urlgrabber.progress as progress
> -import util
> +import _util

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.

<snip>

> diff --git a/virtinst/util.py b/virtinst/util.py
> --- a/virtinst/util.py
> +++ b/virtinst/util.py
> @@ -1,5 +1,3 @@
> -#
> -# Utility functions used for guest installation
>  #
>  # Copyright 2006  Red Hat, Inc.
>  # Jeremy Katz <katzj redhat com>
> @@ -19,107 +17,60 @@
>  # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>  # MA 02110-1301 USA.
>  
> +#
> +# WARNING: this file sadly forms part of the public API. Do not add
> +# anything to this file! The file for internal utility functions is
> +# _util.py
> +#
> +
>  import platform
> -import random
> -import os.path
> -import re
> -import libxml2
> -import logging
> -from sys import stderr
> +import os
>  
> -import libvirt
> -from virtinst import _virtinst as _
> -from virtinst import CapabilitiesParser
> +from virtinst import _util
>  
>  
>  KEYBOARD_DIR = "/etc/sysconfig/keyboard"
>  XORG_CONF = "/etc/X11/xorg.conf"
>  
> -def default_route():
> -    route_file = "/proc/net/route"
> -    d = file(route_file)
> +default_route = _util.default_route
> +default_bridge = _util.default_bridge
> +default_network = _util.default_network
> +default_connection = _util.default_connection
> +get_cpu_flags = _util.get_cpu_flags
> +is_pae_capable = _util.is_pae_capable
> +is_blktap_capable = _util.is_blktap_capable
> +get_default_arch = _util.get_default_arch
> +randomMAC = _util.randomMAC
> +randomUUID = _util.randomUUID
> +uuidToString = _util.uuidToString
> +uuidFromString = _util.uuidFromString
> +get_host_network_devices = _util.get_host_network_devices
> +get_max_vcpus = _util.get_max_vcpus
> +get_phy_cpus = _util.get_phy_cpus
> +xml_escape = _util.xml_escape
> +compareMAC = _util.compareMAC
> +default_keymap = _util.default_keymap
> +pygrub_path = _util.pygrub_path
> +uri_split = _util.uri_split
> +is_uri_remote = _util.is_uri_remote
> +get_uri_hostname = _util.get_uri_hostname
> +get_uri_transport = _util.get_uri_transport
> +get_uri_driver = _util.get_uri_driver
> +is_storage_capable = _util.is_storage_capable
> +get_xml_path = _util.get_xml_path
> +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.

That would mean 'util' would stay the same. '_util' would have whatever
new functions you are adding, and the equivalent of the above block of
code. Anyone needing a utility function for just the library will add
it to _util, and if we ever want to make a private utility public,
just move it to 'util'.

Am I missing some benefit of the above approach? Or are we just on
different pages? (My intention really isn't to be a pest :) )

Thanks,
Cole


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