From: Vratko Polak <vrpolak@cisco.com>
Date: Wed, 4 Dec 2019 12:24:07 +0000 (+0100)
Subject: Deal with some "pylint: disable=" comments
X-Git-Url: https://gerrit.fd.io/r/gitweb?p=csit.git;a=commitdiff_plain;h=063abf35e81deaf749ebbcfee339fbd1d9e89412

Deal with some "pylint: disable=" comments

+ When possible, fix the violation.
+ Else, add a comment:
  + An explanation (if not already present) and keep disable.
  + A TODO (if not already present) and remove the disable.
- This makes tox job report more pylint violations,
  but any such violation is fixable and should be fixed.
  - Although some need to be fixed in VPP, such as enum item long names.

Change-Id: I48604b5eda070083d79dff1439620dbd9e798e1f
Signed-off-by: Vratko Polak <vrpolak@cisco.com>
---

diff --git a/pylint.cfg b/pylint.cfg
index 5ae4ed551b..82b3a06215 100644
--- a/pylint.cfg
+++ b/pylint.cfg
@@ -42,8 +42,9 @@ extension-pkg-whitelist=numpy, scipy
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use"--disable=all --enable=classes
 # --disable=W"
-disable=redefined-variable-type, locally-disabled, locally-enabled
+#disable=redefined-variable-type, locally-disabled, locally-enabled
 
+# TODO: Add explanation when disabling an id, either locally or globally.
 
 [REPORTS]
 
diff --git a/resources/libraries/python/ContainerUtils.py b/resources/libraries/python/ContainerUtils.py
index fc61eea3bd..74add98359 100644
--- a/resources/libraries/python/ContainerUtils.py
+++ b/resources/libraries/python/ContainerUtils.py
@@ -11,15 +11,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# Bug workaround in pylint for abstract classes.
-# pylint: disable=W0223
-
 """Library to manipulate Containers."""
 
 from collections import OrderedDict, Counter
 from io import open
 from string import Template
 
+from robot.libraries.BuiltIn import BuiltIn
+
 from resources.libraries.python.Constants import Constants
 from resources.libraries.python.ssh import SSH
 from resources.libraries.python.topology import Topology, SocketType
@@ -442,8 +441,6 @@ class ContainerEngine:
         )
         self.execute(u"supervisorctl start vpp")
 
-        # pylint: disable=import-outside-toplevel
-        from robot.libraries.BuiltIn import BuiltIn
         topo_instance = BuiltIn().get_library_instance(
             u"resources.libraries.python.topology.Topology"
         )
@@ -652,6 +649,13 @@ class LXC(ContainerEngine):
 
         self._configure_cgroup(u"lxc")
 
+    def build(self):
+        """Build container (compile).
+
+        TODO: Remove from parent class if no sibling implements this.
+        """
+        raise NotImplementedError
+
     def create(self):
         """Create/deploy an application inside a container on system.
 
@@ -870,6 +874,13 @@ class Docker(ContainerEngine):
         if self.container.cpuset_cpus:
             self._configure_cgroup(u"docker")
 
+    def build(self):
+        """Build container (compile).
+
+        TODO: Remove from parent class if no sibling implements this.
+        """
+        raise NotImplementedError
+
     def create(self):
         """Create/deploy container.
 
diff --git a/resources/libraries/python/GBP.py b/resources/libraries/python/GBP.py
index 3d4d249b10..59dd269d95 100644
--- a/resources/libraries/python/GBP.py
+++ b/resources/libraries/python/GBP.py
@@ -43,8 +43,9 @@ class GBPBridgeDomainFlags(IntEnum):
 class GBPSubnetType(IntEnum):
     """GBP Subnet Type."""
     GBP_API_SUBNET_TRANSPORT = 1
-    GBP_API_SUBNET_STITCHED_INTERNAL = 2  # pylint: disable=invalid-name
-    GBP_API_SUBNET_STITCHED_EXTERNAL = 3  # pylint: disable=invalid-name
+    # TODO: Names too long for pylint, fix in VPP.
+    GBP_API_SUBNET_STITCHED_INTERNAL = 2
+    GBP_API_SUBNET_STITCHED_EXTERNAL = 3
     GBP_API_SUBNET_L3_OUT = 4
     GBP_API_SUBNET_ANON_L3_OUT = 5
 
diff --git a/resources/libraries/python/IPUtil.py b/resources/libraries/python/IPUtil.py
index f99deb1e08..04961fa45a 100644
--- a/resources/libraries/python/IPUtil.py
+++ b/resources/libraries/python/IPUtil.py
@@ -56,7 +56,8 @@ class FibPathType(IntEnum):
 class FibPathFlags(IntEnum):
     """FIB path flags."""
     FIB_PATH_FLAG_NONE = 0
-    FIB_PATH_FLAG_RESOLVE_VIA_ATTACHED = 1  # pylint: disable=invalid-name
+    # TODO: Name too long for pylint, fix in VPP.
+    FIB_PATH_FLAG_RESOLVE_VIA_ATTACHED = 1
     FIB_PATH_FLAG_RESOLVE_VIA_HOST = 2
 
 
diff --git a/resources/libraries/python/OptionString.py b/resources/libraries/python/OptionString.py
index 9a30d37b9a..bdb5ee2b4c 100644
--- a/resources/libraries/python/OptionString.py
+++ b/resources/libraries/python/OptionString.py
@@ -92,12 +92,17 @@ class OptionString:
         self.parts.extend(other.parts)
         return self
 
-    def _check_and_add(self, part, prefixed):
+    def check_and_add(self, part, prefixed):
         """Convert to string, strip, conditionally add prefixed if non-empty.
 
         Value of None is converted to empty string.
         Emptiness is tested before adding prefix.
 
+        This could be a protected method (name starting with underscore),
+        but then pylint does not understand add_equals and add_with_value
+        are allowed to call this on the temp instance.
+        TODO: Is there a way to make pylint understand?
+
         :param part: Unchecked part to add to list of parts.
         :param prefixed: Whether to add prefix when adding.
         :type part: object
@@ -123,7 +128,7 @@ class OptionString:
         :returns: Self, to enable method chaining.
         :rtype: OptionString
         """
-        self._check_and_add(parameter, prefixed=True)
+        self.check_and_add(parameter, prefixed=True)
         return self
 
     def add_if(self, parameter, condition):
@@ -160,11 +165,8 @@ class OptionString:
         :rtype: OptionString
         """
         temp = OptionString(prefix=self.prefix)
-        # TODO: Is pylint really that ignorant?
-        # How could it not understand temp is of type of this class?
-        # pylint: disable=protected-access
-        if temp._check_and_add(parameter, prefixed=True):
-            if temp._check_and_add(value, prefixed=False):
+        if temp.check_and_add(parameter, prefixed=True):
+            if temp.check_and_add(value, prefixed=False):
                 self.extend(temp)
         return self
 
@@ -183,9 +185,8 @@ class OptionString:
         :rtype: OptionString
         """
         temp = OptionString(prefix=self.prefix)
-        # pylint: disable=protected-access
-        if temp._check_and_add(parameter, prefixed=True):
-            if temp._check_and_add(value, prefixed=False):
+        if temp.check_and_add(parameter, prefixed=True):
+            if temp.check_and_add(value, prefixed=False):
                 self.parts.append(u"=".join(temp.parts))
         return self
 
diff --git a/resources/libraries/python/PacketVerifier.py b/resources/libraries/python/PacketVerifier.py
index 397ce76f49..fb2337e49d 100644
--- a/resources/libraries/python/PacketVerifier.py
+++ b/resources/libraries/python/PacketVerifier.py
@@ -75,7 +75,6 @@ from scapy.packet import Raw
 
 # Enable libpcap's L2listen
 conf.use_pcap = True
-import scapy.arch.pcapdnet  # pylint: disable=C0413, unused-import
 
 __all__ = [
     u"RxQueue", u"TxQueue", u"Interface", u"create_gratuitous_arp_request",
@@ -235,7 +234,11 @@ class RxQueue(PacketVerifier):
             pkt_pad = str(auto_pad(pkt))
             print(f"Received packet on {self._ifname} of len {len(pkt)}")
             if verbose:
-                pkt.show2()  # pylint: disable=no-member
+                if hasattr(pkt, u"show2"):
+                    pkt.show2()
+                else:
+                    # Never happens in practice, but Pylint does not know that.
+                    print(f"Unexpected instance: {pkt!r}")
                 print()
             if pkt_pad in ignore_list:
                 ignore_list.remove(pkt_pad)
diff --git a/resources/libraries/python/PapiExecutor.py b/resources/libraries/python/PapiExecutor.py
index 7f226f4ff3..5a79a60903 100644
--- a/resources/libraries/python/PapiExecutor.py
+++ b/resources/libraries/python/PapiExecutor.py
@@ -196,6 +196,11 @@ class PapiSocketExecutor:
             # Package path has to be one level above the vpp_papi directory.
             package_path = package_path.rsplit(u"/", 1)[0]
             sys.path.append(package_path)
+            # Only now, interpreter has a chance to locate the code to import.
+            # That means the import statement here is in the correct place.
+            # No refactor allows the import to be moved to where pylint wants,
+            # and pylint does not execute the logic for locating the code,
+            # so, dear pylint, please ignore these offences.
             # pylint: disable=import-outside-toplevel, import-error
             from vpp_papi.vpp_papi import VPPApiClient as vpp_class
             vpp_class.apidir = api_json_directory
diff --git a/resources/libraries/python/Policer.py b/resources/libraries/python/Policer.py
index f6a852e417..d5500e1d89 100644
--- a/resources/libraries/python/Policer.py
+++ b/resources/libraries/python/Policer.py
@@ -84,7 +84,11 @@ class DSCP(IntEnum):
 class Policer:
     """Policer utilities."""
 
-    # pylint: disable=too-many-arguments, too-many-locals
+    # TODO: Pylint says too-many-arguments and too-many-locals.
+    # It is right, we should refactor the code
+    # and group similar arguments together (into documented classes).
+    # Note that even the call from Robot Framework
+    # is not very readable with this many arguments.
     @staticmethod
     def policer_set_configuration(
             node, policer_name, cir, eir, cbs, ebs, rate_type, round_type,
diff --git a/resources/libraries/python/TrafficGenerator.py b/resources/libraries/python/TrafficGenerator.py
index 14d2dc8d1c..007079f254 100644
--- a/resources/libraries/python/TrafficGenerator.py
+++ b/resources/libraries/python/TrafficGenerator.py
@@ -125,7 +125,8 @@ class TGDropRateSearchImpl(DropRateSearch):
         return tg_instance.get_latency_int()
 
 
-# pylint: disable=too-many-instance-attributes
+# TODO: Pylint says too-many-instance-attributes.
+# A fix is developed in https://gerrit.fd.io/r/c/csit/+/22221
 class TrafficGenerator(AbstractMeasurer):
     """Traffic Generator.
 
@@ -201,7 +202,8 @@ class TrafficGenerator(AbstractMeasurer):
         """
         return self._latency
 
-    # pylint: disable=too-many-locals
+    # TODO: pylint says disable=too-many-locals.
+    # A fix is developed in https://gerrit.fd.io/r/c/csit/+/22221
     def initialize_traffic_generator(
             self, tg_node, tg_if1, tg_if2, tg_if1_adj_node, tg_if1_adj_if,
             tg_if2_adj_node, tg_if2_adj_if, osi_layer, tg_if1_dst_mac=None,
diff --git a/resources/libraries/python/VatExecutor.py b/resources/libraries/python/VatExecutor.py
index be8dbbe9dc..19b92a6a1a 100644
--- a/resources/libraries/python/VatExecutor.py
+++ b/resources/libraries/python/VatExecutor.py
@@ -20,6 +20,8 @@ from os import remove
 from paramiko.ssh_exception import SSHException
 from robot.api import logger
 
+import resources.libraries.python.DUTSetup as PidLib
+
 from resources.libraries.python.Constants import Constants
 from resources.libraries.python.PapiHistory import PapiHistory
 from resources.libraries.python.ssh import SSH, SSHTimeout
@@ -60,8 +62,6 @@ def get_vpp_pid(node):
         running on the DUT node.
     :rtype: int or list
     """
-    # pylint: disable=import-outside-toplevel
-    import resources.libraries.python.DUTSetup as PidLib
     pid = PidLib.DUTSetup.get_vpp_pid(node)
     return pid
 
diff --git a/resources/libraries/python/topology.py b/resources/libraries/python/topology.py
index 46a6628a0a..92ade4a7a3 100644
--- a/resources/libraries/python/topology.py
+++ b/resources/libraries/python/topology.py
@@ -41,15 +41,19 @@ def load_topo_from_yaml():
         return safe_load(work_file.read())[u"nodes"]
 
 
-# pylint: disable=invalid-name
 
 class NodeType:
     """Defines node types used in topology dictionaries."""
+    # TODO: Two letter initialisms are well-known, but too short for pylint.
+    # Candidates: TG -> TGN, VM -> VNF.
+
     # Device Under Test (this node has VPP running on it)
     DUT = u"DUT"
     # Traffic Generator (this node has traffic generator on it)
+    # pylint: disable=invalid-name
     TG = u"TG"
     # Virtual Machine (this node running on DUT node)
+    # pylint: disable=invalid-name
     VM = u"VM"
 
 
diff --git a/resources/traffic_scripts/ipsec.py b/resources/traffic_scripts/ipsec.py
index 320303d1fb..9992a7c8cd 100755
--- a/resources/traffic_scripts/ipsec.py
+++ b/resources/traffic_scripts/ipsec.py
@@ -19,9 +19,6 @@ import sys
 import logging
 
 from ipaddress import ip_address
-# pylint: disable=no-name-in-module
-# pylint: disable=import-error
-logging.getLogger(u"scapy.runtime").setLevel(logging.ERROR)
 from scapy.layers.inet import IP
 from scapy.layers.inet6 import IPv6, ICMPv6ND_NS
 from scapy.layers.ipsec import SecurityAssociation, ESP
@@ -122,8 +119,7 @@ def check_ip(pkt_recv, ip_layer, src_ip, dst_ip):
         )
 
 
-# pylint: disable=too-many-locals
-# pylint: disable=too-many-statements
+# TODO: Pylint says too-many-locals and too-many-statements. Refactor!
 def main():
     """Send and receive IPsec packet."""
 
diff --git a/resources/traffic_scripts/policer.py b/resources/traffic_scripts/policer.py
index c222c76152..97cad612ac 100755
--- a/resources/traffic_scripts/policer.py
+++ b/resources/traffic_scripts/policer.py
@@ -19,10 +19,6 @@ import sys
 import logging
 
 from ipaddress import ip_address
-# pylint: disable=no-name-in-module
-# pylint: disable=import-error
-logging.getLogger("scapy.runtime").setLevel(logging.ERROR)
-
 from scapy.layers.l2 import Ether
 from scapy.layers.inet import IP, TCP
 from scapy.layers.inet6 import IPv6, ICMPv6ND_NS
@@ -72,8 +68,7 @@ def check_ipv6(pkt_recv, dscp):
         raise RuntimeError(f"Not a TCP packet received: {pkt_recv!r}")
 
 
-# pylint: disable=too-many-locals
-# pylint: disable=too-many-statements
+# TODO: Pylint says too-many-locals and too-many-statements. Refactor!
 def main():
     """Send and receive TCP packet."""
     args = TrafficScriptArg(