Nir Soffer has posted comments on this change. Change subject: cleanup: Improve networking imports (PEP328) ......................................................................
Patch Set 1: Code-Review-1 (16 comments) Important cleanup - but mixes many unrelated cleanups. This patch does much more than promised in the commit message: - Merge multiple imports into one line - this change is wrong, as it makes reviewing import changes harder. If you want to push this evil practice, do this in another patch. - Change import order - not sure why this was done - Add missing copyright - Fix the years in another copyright Please remove anything which is not related to absolute imports. .................................................... Commit Message Line 29: from . import moduleX Line 30: from ..subpackage2 import moduleZ Line 31: Line 32: Which makes it much more evident where things are and avoids possible Line 33: shadowing of packages in site-packages. Great commit message! I wish we have more like this. Line 34: Line 35: Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 .................................................... File lib/vdsm/netinfo.py Line 28: import logging Line 29: import os Line 30: import shlex Line 31: import socket Line 32: import struct Why did you change the order of imports? How is this related to absolute imports? Line 33: Line 34: import ethtool Line 35: Line 36: from .config import config Line 35: Line 36: from .config import config Line 37: from . import constants Line 38: from . import libvirtconnection Line 39: from .ipwrapper import linkShowDev, Route, routeShowAllDefaultGateways How is merging multiple imports to one line import related to the purpose of this patch? Line 40: Line 41: NET_CONF_DIR = '/etc/sysconfig/network-scripts/' Line 42: # ifcfg persistence directories Line 43: NET_CONF_BACK_DIR = constants.P_VDSM_LIB + 'netconfback/' .................................................... File vdsm/configNetwork.py Line 29: from . import hooks Line 30: from . import neterrors as ne Line 31: from .netconf.ifcfg import ConfigWriter, Ifcfg Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan How is merging multiple import lines to one import line related to the purpose of this patch? Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils Line 36: from vdsm.utils import execCmd Line 37: Line 31: from .netconf.ifcfg import ConfigWriter, Ifcfg Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils How is merging multiple import lines to one import line related to the purpose of this patch? Line 36: from vdsm.utils import execCmd Line 37: Line 38: CONNECTIVITY_TIMEOUT_DEFAULT = 4 Line 39: Line 32: from .neterrors import ConfigNetworkError Line 33: from .netmodels import Bond, Bridge, IPv4, IPv6, IpConfig, Nic, Vlan Line 34: from vdsm.config import config Line 35: from vdsm import constants, netinfo, utils Line 36: from vdsm.utils import execCmd vdsm.utils.execCmd is *not* storage.misc.execCmd! Line 37: Line 38: CONNECTIVITY_TIMEOUT_DEFAULT = 4 Line 39: Line 40: .................................................... File vdsm/netconf/__init__.py Line 20: Line 21: import logging Line 22: Line 23: from ..netmodels import Bond, Bridge Line 24: from ..sourceRoute import DynamicSourceRoute, StaticSourceRoute How is merging multiple import lines to one import line related to the purpose of this patch? Line 25: from vdsm.config import config Line 26: from vdsm import netinfo Line 27: from vdsm.netconfpersistence import RunningConfig Line 28: Line 22: Line 23: from ..netmodels import Bond, Bridge Line 24: from ..sourceRoute import DynamicSourceRoute, StaticSourceRoute Line 25: from vdsm.config import config Line 26: from vdsm import netinfo Why did you change the order of imports? Line 27: from vdsm.netconfpersistence import RunningConfig Line 28: Line 29: Line 30: class Configurator(object): .................................................... File vdsm/netconf/iproute2.py Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: Line 20: Line 21: from vdsm.ipwrapper import routeAdd, routeDel, ruleAdd, ruleDel This style makes it harder to review when import are changed. One import per line is a better strategy. Line 22: Line 23: Line 24: class Iproute2(object): Line 25: @staticmethod .................................................... File vdsm/netconf/libvirtCfg.py Line 19: import libvirt Line 20: from xml.dom.minidom import Document Line 21: from xml.sax.saxutils import escape Line 22: Line 23: from vdsm import libvirtconnection, netinfo How is merging multiple import lines to one import line related to the purpose of this patch? Line 24: Line 25: Line 26: def flush(): Line 27: conn = libvirtconnection.get() .................................................... File vdsm/sourceRoute.py Line 20: import logging Line 21: import os Line 22: Line 23: from libvirt import libvirtError Line 24: import netaddr Why did you change the order of imports and how is this related to this patch? Line 25: Line 26: from vdsm.constants import P_VDSM_RUN Line 27: from vdsm import netinfo Line 28: from vdsm.ipwrapper import IPRoute2Error, Route, routeShowTable, Rule, ruleList Line 24: import netaddr Line 25: Line 26: from vdsm.constants import P_VDSM_RUN Line 27: from vdsm import netinfo Line 28: from vdsm.ipwrapper import IPRoute2Error, Route, routeShowTable, Rule, ruleList How is merging multiple import lines to one import line related to the purpose of this patch? Line 29: from vdsm.utils import rmFile Line 30: Line 31: Line 32: TRACKED_INTERFACES_FOLDER = P_VDSM_RUN + 'trackedInterfaces' .................................................... File vdsm/sourceRouteThread.py Line 16: # Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: from __future__ import absolute_import Line 20: How is this copyright notice related to this patch? Line 21: import logging Line 22: import os Line 23: Line 24: import pyinotify .................................................... File vdsm/tc.py Line 26: Line 27: import ethtool Line 28: Line 29: from vdsm.constants import EXT_TC Line 30: from vdsm.utils import execCmd storage.misc.execCmd is *not* vdsm.utils.execCmd! Line 31: Line 32: ERR_DEV_NOEXIST = 2 Line 33: Line 34: QDISC_INGRESS = 'ffff:' .................................................... File vdsm/vdsm-restore-net-config Line 1: #! /usr/bin/python Line 2: # Line 3: # Copyright 2011-2013 Red Hat, Inc. How is this related to this patch? Line 4: # Line 5: # This program is free software; you can redistribute it and/or modify Line 6: # it under the terms of the GNU General Public License as published by Line 7: # the Free Software Foundation; either version 2 of the License, or Line 25: Line 26: from .configNetwork import setupNetworks Line 27: from .netconf import ifcfg Line 28: from vdsm.config import config Line 29: from vdsm.netconfpersistence import RunningConfig, PersistentConfig Why did you change the import order? Line 30: Line 31: Line 32: def ifcfg_restoration(): Line 33: configWriter = ifcfg.ConfigWriter() -- To view, visit http://gerrit.ovirt.org/20555 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I249cfa0ad734ea45ecbbecbade9daeed6c3adc12 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
