Giuseppe Vallarelli has posted comments on this change. Change subject: iproute2 binary wrapper ......................................................................
Patch Set 8: I would prefer that you didn't submit this (7 inline comments) .................................................... File vdsm/ipwrapper.py Line 16: # Line 17: # Refer to the README and COPYING files for full details of the license Line 18: # Line 19: Line 20: from vdsm import utils See http://www.python.org/dev/peps/pep-0008/#imports for grouping imports. Line 21: import netaddr Line 22: from collections import defaultdict Line 23: Line 24: Line 21: import netaddr Line 22: from collections import defaultdict Line 23: Line 24: Line 25: ipBinary = str(utils.CommandPath("ip", "/sbin/ip")) See http://www.python.org/dev/peps/pep-0008/#constants for constants. Line 26: Line 27: Line 28: def _isValid(ip, verifier): Line 29: try: Line 24: Line 25: ipBinary = str(utils.CommandPath("ip", "/sbin/ip")) Line 26: Line 27: Line 28: def _isValid(ip, verifier): You don't need to have 3 functions, one should be enough, the subject under validation is already clear when you call the function with the specified verifier, for example: isValidIp(ip, netaddr.IPAddress), isValidIp(ip, netaddr.IPNetwork). Line 29: try: Line 30: verifier(ip) Line 31: except (netaddr.core.AddrFormatError, ValueError): Line 32: return False Line 42: return _isValid(ipnetwork, netaddr.IPNetwork) Line 43: Line 44: Line 45: class Route(object): Line 46: def __init__(self, network=None, ipaddr=None, device=None, table=None): Route's constructor tells me that is perfectly fine to create an object without specifying any params, but later you tax the user of this class by checking if network is None.. Short answer network should be always provided. Line 47: if network is None: Line 48: raise ValueError("network is required") Line 49: Line 50: if not _isValidNetwork(network): Line 46: def __init__(self, network=None, ipaddr=None, device=None, table=None): Line 47: if network is None: Line 48: raise ValueError("network is required") Line 49: Line 50: if not _isValidNetwork(network): You can make this dependency explicit: class Route(object): def __init__(self, network=None, ipaddr=None, device=None, table=None, verifier=validateIp): Later in the body you can have: validateIp(network, netaddr.IPNetwork) and validateIp(ipaddr, netaddr.IPAddress). validateIP can encapsulate the raising of the exception this way code will look cleaner. At the same time, Route will be easier to test having this dependency explicit. If you're interested in knowing more google for dependency injection. Line 51: raise ValueError("network is not properly defined") Line 52: Line 53: if ipaddr and not _isValidIP(ipaddr): Line 54: raise ValueError("ipaddr is not properly defined") Line 98: yield word Line 99: Line 100: Line 101: class Rule(object): Line 102: def __init__(self, source=None, destination=None, table=None): Same consideration of the Route's network param applies to table. Line 103: if table is None: Line 104: raise ValueError("table is a required parameter") Line 105: Line 106: if source is not None: Line 162: str += " table %s" % self.table Line 163: Line 164: return str Line 165: Line 166: def __iter__(self): There are different commonalities between Route and Rule like some checks in the fromText classmethods and __iter__, you may think to abstract them away, but it can be overkill for now to add another abstraction. Keep this in mind if more functionalities are added to both objects. One of the most used principles is DRY (don't repeat yourself). Line 167: for word in str(self).split(): Line 168: yield word Line 169: Line 170: -- To view, visit http://gerrit.ovirt.org/15335 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1d315c3294fd7f058cdc840dea329d91a658a304 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com> Gerrit-Reviewer: Livnat Peer <lp...@redhat.com> Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
