Fix Pylint upgrade issues

* Remove unsupported pylint disable options
    * star-args removed in Pylint 1.4.3
    * abstract-class-little-used removed in Pylint 1.4.3

* Fixes new lint errors

* Copy dummy-variable-rgx expression to new ignored-argument-names expression to ignore unused funtion arguments

* Notable changes
    * Refactor to satisfy Pylint no-else-return warning
    * Fix Pylint inconsistent-return-statements warning
    * Refactor to satisfy consider-iterating-dictionary
    * Remove methods with only super call to satisfy useless-super-delegation
    * Refactor too-many-nested-statements where possible
    * Suppress type checked errors where member is dynamically added (notably derived from josepy.JSONObjectWithFields)
    * Remove None default of func parameter for ExitHandler and ErrorHandler

Resolves #5973

certbot-apache/certbot_apache/configurator.py

@@ -3,12 +3,15 @@
 import fnmatch
 import logging
 import os
-import pkg_resources
 import re
-import six
 import socket
 import time
 
+from collections import defaultdict
+
+import pkg_resources
+import six
+
 import zope.component
 import zope.interface
 
@@ -32,8 +35,6 @@ from certbot_apache import obj
 from certbot_apache import parser
 from certbot_apache import tls_sni_01
 
-from collections import defaultdict
-
 logger = logging.getLogger(__name__)
 
 
@@ -214,7 +215,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                          '.'.join(str(i) for i in self.version))
         if self.version < (2, 2):
             raise errors.NotSupportedError(
-                "Apache Version %s not supported.", str(self.version))
+                "Apache Version {0} not supported.".format(str(self.version)))
 
         if not self._check_aug_version():
             raise errors.NotSupportedError(
@@ -245,7 +246,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         except (OSError, errors.LockError):
             logger.debug("Encountered error:", exc_info=True)
             raise errors.PluginError(
-                "Unable to lock %s", self.conf("server-root"))
+                "Unable to lock {0}".format(self.conf("server-root")))
 
     def _check_aug_version(self):
         """ Checks that we have recent enough version of libaugeas.
@@ -352,6 +353,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
         """
         if len(name.split(".")) == len(domain.split(".")):
             return fnmatch.fnmatch(name, domain)
+        return None
 
 
     def _choose_vhosts_wildcard(self, domain, create_ssl=True):
@@ -678,7 +680,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                     if name:
                         all_names.add(name)
 
-        if len(vhost_macro) > 0:
+        if vhost_macro:
             zope.component.getUtility(interfaces.IDisplay).notification(
                 "Apache mod_macro seems to be in use in file(s):\n{0}"
                 "\n\nUnfortunately mod_macro is not yet supported".format(
@@ -1024,6 +1026,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
                 # Ugly but takes care of protocol def, eg: 1.1.1.1:443 https
                 if listen.split(":")[-1].split(" ")[0] == port:
                     return True
+        return None
 
     def prepare_https_modules(self, temp):
         """Helper method for prepare_server_https, taking care of enabling
@@ -1170,8 +1173,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
         if fp.endswith(".conf"):
             return fp[:-(len(".conf"))] + self.conf("le_vhost_ext")
-        else:
-            return fp + self.conf("le_vhost_ext")
+        return fp + self.conf("le_vhost_ext")
 
     def _sift_rewrite_rule(self, line):
         """Decides whether a line should be copied to a SSL vhost.
@@ -1391,8 +1393,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator):
 
     def _remove_directives(self, vh_path, directives):
         for directive in directives:
-            while len(self.parser.find_dir(directive, None,
-                                           vh_path, False)) > 0:
+            while self.parser.find_dir(directive, None, vh_path, False):
                 directive_path = self.parser.find_dir(directive, None,
                                                       vh_path, False)
                 self.aug.remove(re.sub(r"/\w*$", "", directive_path[0]))

certbot-apache/certbot_apache/display_ops.py

@@ -26,7 +26,7 @@ def select_vhost_multiple(vhosts):
         return list()
     tags_list = [vhost.display_repr()+"\n" for vhost in vhosts]
     # Remove the extra newline from the last entry
-    if len(tags_list):
+    if tags_list:
         tags_list[-1] = tags_list[-1][:-1]
     code, names = zope.component.getUtility(interfaces.IDisplay).checklist(
         "Which VirtualHosts would you like to install the wildcard certificate for?",
@@ -62,8 +62,7 @@ def select_vhost(domain, vhosts):
     code, tag = _vhost_menu(domain, vhosts)
     if code == display_util.OK:
         return vhosts[tag]
-    else:
-        return None
+    return None
 
 def _vhost_menu(domain, vhosts):
     """Select an appropriate Apache Vhost.
@@ -93,7 +92,7 @@ def _vhost_menu(domain, vhosts):
     for vhost in vhosts:
         if len(vhost.get_names()) == 1:
             disp_name = next(iter(vhost.get_names()))
-        elif len(vhost.get_names()) == 0:
+        elif not vhost.get_names():
             disp_name = ""
         else:
             disp_name = "Multiple Names"

certbot-apache/certbot_apache/obj.py

@@ -26,7 +26,7 @@ class Addr(common.Addr):
     def __repr__(self):
         return "certbot_apache.obj.Addr(" + repr(self.tup) + ")"
 
-    def __hash__(self):
+    def __hash__(self):  # pylint: disable=useless-super-delegation
         # Python 3 requires explicit overridden for __hash__ if __eq__ or
         # __cmp__ is overridden. See https://bugs.python.org/issue2235
         return super(Addr, self).__hash__()
@@ -47,8 +47,7 @@ class Addr(common.Addr):
             return 0
         elif self.get_addr() == "*":
             return 1
-        else:
-            return 2
+        return 2
 
     def conflicts(self, addr):
         r"""Returns if address could conflict with correct function of self.

certbot-apache/certbot_apache/override_centos.py

@@ -56,5 +56,5 @@ class CentOSParser(parser.ApacheParser):
     def parse_sysconfig_var(self):
         """ Parses Apache CLI options from CentOS configuration file """
         defines = apache_util.parse_define_file(self.sysconfig_filep, "OPTIONS")
-        for k in defines.keys():
+        for k in defines:
             self.variables[k] = defines[k]

certbot-apache/certbot_apache/override_debian.py

@@ -51,7 +51,7 @@ class DebianConfigurator(configurator.ApacheConfigurator):
 
         """
         if vhost.enabled:
-            return
+            return None
 
         enabled_path = ("%s/sites-enabled/%s" %
                         (self.parser.root,
@@ -68,7 +68,7 @@ class DebianConfigurator(configurator.ApacheConfigurator):
                enabled_path) == vhost.filep:
                 # Already in shape
                 vhost.enabled = True
-                return
+                return None
             else:
                 logger.warning(
                     "Could not symlink %s to %s, got error: %s", enabled_path,
@@ -81,6 +81,7 @@ class DebianConfigurator(configurator.ApacheConfigurator):
         vhost.enabled = True
         logger.info("Enabling available site: %s", vhost.filep)
         self.save_notes += "Enabled site %s\n" % vhost.filep
+        return None
 
     def enable_mod(self, mod_name, temp=False):
         # pylint: disable=unused-argument

certbot-apache/certbot_apache/override_gentoo.py

@@ -56,7 +56,7 @@ class GentooParser(parser.ApacheParser):
         """ Parses Apache CLI options from Gentoo configuration file """
         defines = apache_util.parse_define_file(self.apacheconfig_filep,
                                                 "APACHE2_OPTS")
-        for k in defines.keys():
+        for k in defines:
             self.variables[k] = defines[k]
 
     def update_modules(self):

certbot-apache/certbot_apache/parser.py

@@ -82,7 +82,7 @@ class ApacheParser(object):
         :param str inc_path: path of file to include
 
         """
-        if len(self.find_dir(case_i("Include"), inc_path)) == 0:
+        if not self.find_dir(case_i("Include"), inc_path):
             logger.debug("Adding Include %s to %s",
                          inc_path, get_aug_path(main_config))
             self.add_dir(
@@ -138,7 +138,7 @@ class ApacheParser(object):
                     mods.add(os.path.basename(mod_filename)[:-2] + "c")
                 else:
                     logger.debug("Could not read LoadModule directive from " +
-                                 "Augeas path: {0}".format(match_name[6:]))
+                                 "Augeas path: %s", match_name[6:])
         self.modules.update(mods)
 
     def update_runtime_variables(self):
@@ -228,8 +228,8 @@ class ApacheParser(object):
                 "Error running command %s for runtime parameters!%s",
                 command, os.linesep)
             raise errors.MisconfigurationError(
-                "Error accessing loaded Apache parameters: %s",
-                command)
+                "Error accessing loaded Apache parameters: {0}".format(
+                command))
         # Small errors that do not impede
         if proc.returncode != 0:
             logger.warning("Error in checking parameter list: %s", stderr)
@@ -255,12 +255,12 @@ class ApacheParser(object):
         """
         filtered = []
         if args == 1:
-            for i in range(len(matches)):
-                if matches[i].endswith("/arg"):
+            for i, match in enumerate(matches):
+                if match.endswith("/arg"):
                     filtered.append(matches[i][:-4])
         else:
-            for i in range(len(matches)):
-                if matches[i].endswith("/arg[%d]" % args):
+            for i, match in enumerate(matches):
+                if match.endswith("/arg[%d]" % args):
                     # Make sure we don't cause an IndexError (end of list)
                     # Check to make sure arg + 1 doesn't exist
                     if (i == (len(matches) - 1) or
@@ -305,7 +305,7 @@ class ApacheParser(object):
         """
         if_mods = self.aug.match(("%s/IfModule/*[self::arg='%s']" %
                                   (aug_conf_path, mod)))
-        if len(if_mods) == 0:
+        if not if_mods:
             self.aug.set("%s/IfModule[last() + 1]" % aug_conf_path, "")
             self.aug.set("%s/IfModule[last()]/arg" % aug_conf_path, mod)
             if_mods = self.aug.match(("%s/IfModule/*[self::arg='%s']" %
@@ -569,9 +569,8 @@ class ApacheParser(object):
         if sys.version_info < (3, 6):
             # This strips off final /Z(?ms)
             return fnmatch.translate(clean_fn_match)[:-7]
-        else:  # pragma: no cover
-            # Since Python 3.6, it returns a different pattern like (?s:.*\.load)\Z
-            return fnmatch.translate(clean_fn_match)[4:-3]
+        # Since Python 3.6, it returns a different pattern like (?s:.*\.load)\Z
+        return fnmatch.translate(clean_fn_match)[4:-3]  # pragma: no cover
 
     def parse_file(self, filepath):
         """Parse file with Augeas
@@ -653,10 +652,7 @@ class ApacheParser(object):
                 use_new = False
             else:
                 use_new = True
-            if new_file_match == "*":
-                remove_old = True
-            else:
-                remove_old = False
+            remove_old = new_file_match == "*"
         except KeyError:
             use_new = True
             remove_old = False

certbot-apache/certbot_apache/tests/configurator_test.py

@@ -340,6 +340,7 @@ class MultipleVhostsTest(util.ApacheTest):
             """Mock method for parser.find_dir"""
             if directive == "Include" and argument.endswith("options-ssl-apache.conf"):
                 return ["/path/to/whatever"]
+            return None  # pragma: no cover
 
         mock_add = mock.MagicMock()
         self.config.parser.add_dir = mock_add
@@ -451,8 +452,7 @@ class MultipleVhostsTest(util.ApacheTest):
             but an SSLCertificateKeyFile directive is missing."""
             if "SSLCertificateFile" in args:
                 return ["example/cert.pem"]
-            else:
-                return []
+            return []
 
         mock_find_dir = mock.MagicMock(return_value=[])
         mock_find_dir.side_effect = side_effect
@@ -1025,7 +1025,7 @@ class MultipleVhostsTest(util.ApacheTest):
 
         # pylint: disable=protected-access
         http_vh = self.config._get_http_vhost(ssl_vh)
-        self.assertTrue(http_vh.ssl == False)
+        self.assertFalse(http_vh.ssl)
 
     @mock.patch("certbot.util.run_script")
     @mock.patch("certbot.util.exe_exists")
@@ -1390,7 +1390,7 @@ class MultipleVhostsTest(util.ApacheTest):
         # pylint: disable=protected-access
         cases = {u"*.example.org": True, b"*.x.example.org": True,
                  u"a.example.org": False, b"a.x.example.org": False}
-        for key in cases.keys():
+        for key in cases:
             self.assertEqual(self.config._wildcard_domain(key), cases[key])
 
     def test_choose_vhosts_wildcard(self):
@@ -1514,7 +1514,7 @@ class AugeasVhostsTest(util.ApacheTest):
     def test_choosevhost_works(self):
         path = "debian_apache_2_4/augeas_vhosts/apache2/sites-available/old,default.conf"
         chosen_vhost = self.config._create_vhost(path)
-        self.assertTrue(chosen_vhost == None or chosen_vhost.path == path)
+        self.assertTrue(chosen_vhost is None or chosen_vhost.path == path)
 
     @mock.patch("certbot_apache.configurator.ApacheConfigurator._create_vhost")
     def test_get_vhost_continue(self, mock_vhost):

certbot-apache/certbot_apache/tests/entrypoint_test.py

@@ -14,7 +14,7 @@ class EntryPointTest(unittest.TestCase):
     def test_get_configurator(self):
 
         with mock.patch("certbot.util.get_os_info") as mock_info:
-            for distro in entrypoint.OVERRIDE_CLASSES.keys():
+            for distro in entrypoint.OVERRIDE_CLASSES:
                 mock_info.return_value = (distro, "whatever")
                 self.assertEqual(entrypoint.get_configurator(),
                                  entrypoint.OVERRIDE_CLASSES[distro])
@@ -23,7 +23,7 @@ class EntryPointTest(unittest.TestCase):
         with mock.patch("certbot.util.get_os_info") as mock_info:
             mock_info.return_value = ("nonexistent", "irrelevant")
             with mock.patch("certbot.util.get_systemd_os_like") as mock_like:
-                for like in entrypoint.OVERRIDE_CLASSES.keys():
+                for like in entrypoint.OVERRIDE_CLASSES:
                     mock_like.return_value = [like]
                     self.assertEqual(entrypoint.get_configurator(),
                                      entrypoint.OVERRIDE_CLASSES[like])

certbot-apache/certbot_apache/tests/gentoo_test.py

@@ -113,6 +113,7 @@ class MultipleVhostsTestGentoo(util.ApacheTest):
             """Mock httpd process stdout"""
             if command == ['apache2ctl', 'modules']:
                 return mod_val
+            return None  # pragma: no cover
         mock_get.side_effect = mock_get_cfg
         self.config.parser.modules = set()
 

certbot-apache/certbot_apache/tests/http_01_test.py

@@ -1,7 +1,7 @@
 """Test for certbot_apache.http_01."""
-import mock
 import os
 import unittest
+import mock
 
 from acme import challenges
 from acme.magic_typing import List  # pylint: disable=unused-import, no-name-in-module
@@ -19,7 +19,7 @@ NUM_ACHALLS = 3
 class ApacheHttp01Test(util.ApacheTest):
     """Test for certbot_apache.http_01.ApacheHttp01."""
 
-    def setUp(self, *args, **kwargs):
+    def setUp(self, *args, **kwargs):  # pylint: disable=arguments-differ
         super(ApacheHttp01Test, self).setUp(*args, **kwargs)
 
         self.account_key = self.rsa512jwk
@@ -77,7 +77,7 @@ class ApacheHttp01Test(util.ApacheTest):
         calls = mock_enmod.call_args_list
         other_calls = []
         for call in calls:
-            if "rewrite" != call[0][0]:
+            if call[0][0] != "rewrite":
                 other_calls.append(call)
 
         # If these lists are equal, we never enabled mod_rewrite

certbot-apache/certbot_apache/tests/parser_test.py

@@ -234,6 +234,7 @@ class BasicParserTest(util.ParserTest):
                 return inc_val
             elif cmd[-1] == "DUMP_MODULES":
                 return mod_val
+            return None  # pragma: no cover
 
         mock_cfg.side_effect = mock_get_vars
 

certbot-apache/certbot_apache/tests/tls_sni_01_test.py

@@ -4,14 +4,14 @@ import unittest
 
 import mock
 
+from six.moves import xrange  # pylint: disable=redefined-builtin, import-error
+
 from certbot import errors
 from certbot.plugins import common_test
 
 from certbot_apache import obj
 from certbot_apache.tests import util
 
-from six.moves import xrange  # pylint: disable=redefined-builtin, import-error
-
 
 class TlsSniPerformTest(util.ApacheTest):
     """Test the ApacheTlsSni01 challenge."""

certbot-apache/certbot_apache/tests/util.py

@@ -115,8 +115,7 @@ def get_apache_configurator(  # pylint: disable=too-many-arguments, too-many-loc
         """Mock default vhost path"""
         if key == "vhost_root":
             return vhost_path
-        else:
-            return orig_os_constant(key)
+        return orig_os_constant(key)
 
     with mock.patch("certbot_apache.configurator.ApacheConfigurator.constant") as mock_cons:
         mock_cons.side_effect = mock_os_constant

certbot-apache/certbot_apache/tls_sni_01.py

@@ -135,7 +135,7 @@ class ApacheTlsSni01(common.TLSSNI01):
             return addrs
 
         for addr in vhost.addrs:
-            if "_default_" == addr.get_addr():
+            if addr.get_addr() == "_default_":
                 addrs.add(default_addr)
             else:
                 addrs.add(