Fixed: Responded to PR comments, also:

* fixed typos, documentation and help improvements
 * factored out sorting and slicing
 * user convenience for ssh
 * avoid being os-specific
 * add return status equal to total violations
 * fixed pylint's path issue causing false import errors

fabfile/README.md

@@ -35,7 +35,7 @@ Furthermore, you can use -- to run arbitrary shell commands rather than tasks:
    Create a virtualenv called pybitmessage and install fabfile/requirements.txt
     $ mkvirtualenv -r fabfile/requirements.txt --system-site-packages pybitmessage-devops
  * Ensure you can ssh localhost with no intervention, which may include:
-   * settings in ~/.ssh/config
+   * ssh [sshd_config server] and [ssh_config client] configuration
    * authorized_keys file
    * load ssh key
    * check(!) and accept the host key
@@ -52,3 +52,35 @@ There are a number of advantages that should benefit us:
  * Tasks can be combined either programmatically or on the commandline and run in series or parallel
  * Whole environments can be managed very effectively in conjunction with a configuration management system
 
+<a name="sshd_config"></a>
+# /etc/ssh/sshd_config
+
+If you're going to be using ssh to connect to localhost you want to avoid weakening your security. The best way of
+doing this is blocking port 22 with a firewall. As a belt and braces approach you can also edit the
+/etc/ssh/sshd_config file to restrict login further:
+
+```
+PubkeyAuthentication no
+
+...
+
+Match ::1
+    PubkeyAuthentication yes
+```
+Adapted from [stackexchange](https://unix.stackexchange.com/questions/406245/limit-ssh-access-to-specific-clients-by-ip-address)
+
+<a name="ssh_config"></a>
+# ~/.ssh/config
+
+Fabric will honour your ~/.ssh/config file for your convenience. Since you will spend more time with this key unlocked
+than others you should use a different key:
+
+```
+Host localhost
+    HostName localhost
+    IdentityFile ~/.ssh/id_rsa_localhost
+
+Host github
+    HostName github.com
+    IdentityFile ~/.ssh/id_rsa_github
+```

fabfile/__init__.py

@@ -4,11 +4,29 @@ run commands like this:
 
     $ fab code_quality
 
-For a list of commands
+For a list of commands:
 
+    $ fab -l
+
+For help on fabric itself:
+
+    $ fab -h
+
+For more help on a particular command
 """
 
+from fabric.api import env
+
 from fabfile.tasks import code_quality
 
 
-__all__ = ["code_quality"]
+# Without this, `fab -l` would display the whole docstring as preamble
+__doc__ = ""
+
+# This list defines which tasks are made available to the user
+__all__ = [
+    "code_quality",
+]
+
+# Honour the user's ssh client configuration
+env.use_ssh_config = True

fabfile/lib.py

@@ -1,16 +1,21 @@
-"""A library of functions and constrants for tasks to make use of."""
+# pylint: disable=not-context-manager
+"""
+A library of functions and constants for tasks to make use of.
+
+"""
 
 import os
 import sys
 
 from fabric.api import run, hide
-from fabric.context_managers import settings
+from fabric.context_managers import settings, shell_env
 from fabvenv import virtualenv
 
 
 FABRIC_ROOT = os.path.dirname(__file__)
 PROJECT_ROOT = os.path.dirname(FABRIC_ROOT)
 VENV_ROOT = os.path.expanduser(os.path.join('~', '.virtualenvs', 'pybitmessage-devops'))
+PYTHONPATH = os.path.join(PROJECT_ROOT, 'src',)
 
 
 def coerce_bool(value):
@@ -49,7 +54,7 @@ def flatten(data):
 def pycodestyle(path_to_file):
     """Run pycodestyle on a file"""
     with virtualenv(VENV_ROOT):
-        with hide('warnings', 'running', 'stdout', 'stderr'):  # pylint: disable=not-context-manager
+        with hide('warnings', 'running', 'stdout', 'stderr'):
             with settings(warn_only=True):
                 return run(
                     'pycodestyle --config={0} {1}'.format(
@@ -65,7 +70,7 @@ def pycodestyle(path_to_file):
 def flake8(path_to_file):
     """Run flake8 on a file"""
     with virtualenv(VENV_ROOT):
-        with hide('warnings', 'running', 'stdout'):  # pylint: disable=not-context-manager
+        with hide('warnings', 'running', 'stdout'):
             with settings(warn_only=True):
                 return run(
                     'flake8 --config={0} {1}'.format(
@@ -81,26 +86,65 @@ def flake8(path_to_file):
 def pylint(path_to_file):
     """Run pylint on a file"""
     with virtualenv(VENV_ROOT):
-        with hide('warnings', 'running', 'stdout', 'stderr'):  # pylint: disable=not-context-manager
+        with hide('warnings', 'running', 'stdout', 'stderr'):
             with settings(warn_only=True):
-                return run(
+                with shell_env(PYTHONPATH=PYTHONPATH):
-                    'pylint --rcfile={0} {1}'.format(
+                    return run(
-                        os.path.join(
+                        'pylint --rcfile={0} {1}'.format(
-                            PROJECT_ROOT,
+                            os.path.join(
-                            'setup.cfg',
+                                PROJECT_ROOT,
+                                'setup.cfg',
+                            ),
+                            path_to_file,
                         ),
-                        path_to_file,
+                    )
-                    ),
-                )
 
 
 def autopep8(path_to_file):
     """Run autopep8 on a file"""
     with virtualenv(VENV_ROOT):
-        with hide('running'):  # pylint: disable=not-context-manager
+        with hide('running'):
             with settings(warn_only=True):
                 return run(
                     "autopep8 --experimental --aggressive --aggressive -i --max-line-length=119 {}".format(
                         path_to_file
                     ),
                 )
+
+
+def get_filtered_pycodestyle_output(path_to_file):
+    """Clean up the raw results for pycodestyle"""
+
+    return [
+        i
+        for i in pycodestyle(path_to_file).split(os.linesep)
+        if i
+    ]
+
+
+def get_filtered_flake8_output(path_to_file):
+    """Clean up the raw results for flake8"""
+
+    return [
+        i
+        for i in flake8(path_to_file).split(os.linesep)
+        if i
+    ]
+
+
+def get_filtered_pylint_output(path_to_file):
+    """Clean up the raw results for pylint"""
+
+    return [
+        i
+        for i in pylint(path_to_file).split(os.linesep)
+        if all([
+            i,
+            not i.startswith(' '),
+            not i.startswith('\r'),
+            not i.startswith('-'),
+            not i.startswith('Y'),
+            not i.startswith('*'),
+            not i.startswith('Using config file'),
+        ])
+    ]

fabfile/tasks.py

@@ -1,5 +1,8 @@
+# pylint: disable=not-context-manager
 """
 Fabric tasks for PyBitmessage devops operations.
+
+# pylint: disable=not-context-manager
 """
 
 import os
@@ -8,83 +11,113 @@ import sys
 from fabric.api import run, task, hide, cd
 from fabvenv import virtualenv
 
-from fabfile.lib import pycodestyle, flake8, pylint, autopep8, PROJECT_ROOT, VENV_ROOT, coerce_bool, flatten
+from fabfile.lib import (
+    autopep8, PROJECT_ROOT, VENV_ROOT, coerce_bool, flatten,
+    get_filtered_pycodestyle_output, get_filtered_flake8_output, get_filtered_pylint_output,
+)
 
 
 def get_tool_results(file_list):
     """Take a list of files and resuln the results of applying the tools"""
+
     results = []
     for path_to_file in file_list:
         result = {}
-
+        result['pycodestyle_violations'] = get_filtered_pycodestyle_output(path_to_file)
-        result['pycodestyle_violations'] = [
+        result['flake8_violations'] = get_filtered_flake8_output(path_to_file)
-            i
+        result['pylint_violations'] = get_filtered_pylint_output(path_to_file)
-            for i in pycodestyle(path_to_file).split("\n")
-            if i
-        ]
-        result['flake8_violations'] = [
-            i
-            for i in flake8(path_to_file).split("\n")
-            if i
-        ]
-        result['pylint_violations'] = [
-            i
-            for i in pylint(path_to_file).split("\n")
-            if all([
-                i,
-                not i.startswith(' '),
-                not i.startswith('\r'),
-                not i.startswith('-'),
-                not i.startswith('Y'),
-                not i.startswith('*'),
-                not i.startswith('Using config file'),
-            ])
-        ]
         result['path_to_file'] = path_to_file
-        result['total_violations'] = sum(
+        result['total_violations'] = sum([
-            [
                 len(result['pycodestyle_violations']),
                 len(result['flake8_violations']),
                 len(result['pylint_violations']),
-            ]
+        ])
-        )
         results.append(result)
     return results
 
 
-def print_item(item, verbose, details):
+def print_results(results, top, verbose, details):
     """Print an item with the appropriate verbosity / detail"""
+
     if verbose:
-        line = "{0} {1} {2} {3} {4}".format(
+        print ''.join(
-            item['total_violations'],
+            [
-            len(item['pycodestyle_violations']),
+                os.linesep,
-            len(item['flake8_violations']),
+                'total pycodestyle flake8 pylint path_to_file',
-            len(item['pylint_violations']),
+                os.linesep,
-            item['path_to_file'],
+            ]
         )
-    else:
-        line = item['path_to_file']
-    print line
 
-    if details:
+    for item in sort_and_slice(results, top):
-        print "pycodestyle:"
-        for detail in flatten(item['pycodestyle_violations']):
-            print detail
-        print
 
-        print "flake8:"
+        if verbose:
-        for detail in flatten(item['flake8_violations']):
+            line = "{0} {1} {2} {3} {4}".format(
-            print detail
+                item['total_violations'],
-        print
+                len(item['pycodestyle_violations']),
+                len(item['flake8_violations']),
+                len(item['pylint_violations']),
+                item['path_to_file'],
+            )
+        else:
+            line = item['path_to_file']
+        print line
 
-        print "pylint:"
+        if details:
-        for detail in flatten(item['pylint_violations']):
+            print "pycodestyle:"
-            print detail
+            for detail in flatten(item['pycodestyle_violations']):
-        print
+                print detail
+            print
+
+            print "flake8:"
+            for detail in flatten(item['flake8_violations']):
+                print detail
+            print
+
+            print "pylint:"
+            for detail in flatten(item['pylint_violations']):
+                print detail
+            print
+
+
+def sort_and_slice(results, top):
+    """Sort dictionary items by the `total_violations` key and honour top"""
+
+    returnables = []
+    for item in sorted(
+            results,
+            reverse=True,
+            key=lambda x: x['total_violations']
+    )[:top]:
+        returnables.append(item)
+    return returnables
+
+
+def generate_file_list(filename):
+    """Return an unfiltered list of absolute paths to the files to act on"""
+
+    with hide('warnings', 'running', 'stdout'):
+        with virtualenv(VENV_ROOT):
+
+            if filename:
+                filename = os.path.abspath(filename)
+                if not os.path.exists(filename):
+                    print "Bad filename, specify a Python file"
+                    sys.exit(1)
+                else:
+                    file_list = [filename]
+
+            else:
+                with cd(PROJECT_ROOT):
+                    file_list = [
+                        os.path.abspath(i.rstrip('\r'))
+                        for i in run('find . -name "*.py"').split(os.linesep)
+                    ]
+
+    return file_list
 
 
 @task
-def code_quality(top=10, verbose=True, details=False, fix=False, filename=None):
+def code_quality(verbose=True, details=False, fix=False, filename=None, top=10):
     """
     Check code quality.
 
@@ -95,13 +128,20 @@ def code_quality(top=10, verbose=True, details=False, fix=False, filename=None):
 
     $ fab -H localhost code_quality
 
-    Options:
+    :param top: Display / fix only the top N violating files, a value of 0 will display / fix all files
-        top: Display / fix only the top violating files, a value of 0 will display / fix all files
+    :type top: int, default 10
-        verbose: Display a header and the counts. Without this you just get the filenames in order
+    :param verbose: Display a header and the counts, without this you just get the filenames in order
-        details: Display the violations one per line after the count/file summary
+    :type verbose: bool, default True
-        fix: Run autopep8 on the displayed file(s)
+    :param details: Display the violations one per line after the count / file summary
-        filename: Rather than analysing all files and displaying / fixing the top N, just analyse / diaply / fix the
+    :type details: bool, default False
-        specified file
+    :param fix: Run autopep8 aggressively on the displayed file(s)
+    :type fix: bool, default False
+    :param filename: Rather than analysing all files and displaying / fixing the top N, just analyse / display / fix
+    the specified file
+    :type filename: string, valid path to a file, default all files in the project
+    :return: This fabric task has an exit status equal to the total number of violations and uses stdio but it does
+    not return anything if you manage to call it successfully from Python
+    :rtype: None
 
     Intended to be temporary until we have improved code quality and have safeguards to maintain it in place.
 
@@ -110,38 +150,16 @@ def code_quality(top=10, verbose=True, details=False, fix=False, filename=None):
     verbose = coerce_bool(verbose)
     details = coerce_bool(details)
     fix = coerce_bool(fix)
-    end = int(top) if int(top) else -1
+    top = int(top) or -1
-
-    with hide('warnings', 'running', 'stdout'):  # pylint: disable=not-context-manager
-        with virtualenv(VENV_ROOT):
-            if filename:
-                filename = os.path.abspath(filename)
-                if not os.path.exists(filename):
-                    print "Bad filename, specify a Python file"
-                    sys.exit(1)
-                else:
-                    file_list = [filename]
-            else:
-                with cd(PROJECT_ROOT):  # pylint: disable=not-context-manager
-                    file_list = [
-                        os.path.abspath(i.rstrip('\r'))
-                        for i in run('find . -name "*.py"').split("\n")
-                    ]
-
-    if fix:
-        for path_to_file in file_list:
-            autopep8(path_to_file)
 
+    file_list = generate_file_list(filename)
     results = get_tool_results(file_list)
 
-    if verbose:
+    if fix:
-        print "\ntotal pycodestyle flake8 pylint path_to_file\n"
+        for item in sort_and_slice(results, top):
+            autopep8(item['path_to_file'])
+    # Recalculate results after autopep8 to surprise the user the least
+    results = get_tool_results(file_list)
 
-    # Sort and slice
+    print_results(results, top, verbose, details)
-    for item in sorted(
+    sys.exit(sum([item['total_violations'] for item in results]))
-            results,
-            reverse=True,
-            key=lambda x: x['total_violations']
-    )[:end]:
-
-        print_item(item, verbose, details)

src/bitmessageqt/__init__.py

@@ -1,12 +1,12 @@
 # pylint: disable=too-many-lines,broad-except,too-many-instance-attributes,global-statement,too-few-public-methods
 # pylint: disable=too-many-statements,too-many-branches,attribute-defined-outside-init,too-many-arguments,no-member
-# pylint: disable=unused-argument,no-self-use,too-many-locals,unused-variable,too-many-nested-blocks,ungrouped-imports
+# pylint: disable=unused-argument,no-self-use,too-many-locals,unused-variable,too-many-nested-blocks
 # pylint: disable=too-many-return-statements,protected-access,super-init-not-called,non-parent-init-called
 """
 Initialise the QT interface
 """
 
-from src.debug import logger  # pylint: disable=wrong-import-order
+from debug import logger  # pylint: disable=wrong-import-order
 
 import hashlib
 import locale
@@ -34,38 +34,49 @@ except ImportError:
 
 from sqlite3 import register_adapter
 
-from src import (
+import debug  # pylint: disable=ungrouped-imports
-    shared, defaults, queues, shutdown, state, openclpow, knownnodes, paths, l10n, helper_search, debug, upnp,
+import defaults
-)
+import helper_search
-from src.bitmessageqt import sound, support, dialogs
+import knownnodes
-from src.bitmessageqt.foldertree import (
+import l10n
+import openclpow
+import paths
+import queues
+import shared
+import shutdown
+import state
+import upnp
+
+from bitmessageqt import sound, support, dialogs
+from bitmessageqt.foldertree import (
     AccountMixin, Ui_FolderWidget, Ui_AddressWidget, Ui_SubscriptionWidget, MessageList_AddressWidget,
     MessageList_SubjectWidget, Ui_AddressBookWidgetItemLabel, Ui_AddressBookWidgetItemAddress,
 )
-from src.bitmessageqt.account import (
+from bitmessageqt.account import (
     getSortedAccounts, getSortedSubscriptions, accountClass, BMAccount, GatewayAccount, MailchuckAccount, AccountColor,
 )
-from src.bitmessageqt.bitmessageui import Ui_MainWindow, settingsmixin
+from bitmessageqt.bitmessageui import Ui_MainWindow, settingsmixin
-from src.bitmessageqt.messageview import MessageView
+from bitmessageqt.messageview import MessageView
-from src.bitmessageqt.migrationwizard import Ui_MigrationWizard
+from bitmessageqt.migrationwizard import Ui_MigrationWizard
-from src.bitmessageqt.settings import Ui_settingsDialog
+from bitmessageqt.settings import Ui_settingsDialog
-from src.bitmessageqt.utils import str_broadcast_subscribers, avatarize
+from bitmessageqt.utils import str_broadcast_subscribers, avatarize
-from src.bitmessageqt.uisignaler import UISignaler
+from bitmessageqt.uisignaler import UISignaler
-from src.bitmessageqt.statusbar import BMStatusBar
+from bitmessageqt.statusbar import BMStatusBar
-from src.proofofwork import getPowType
+
-from src.tr import _translate
+from addresses import decodeAddress, addBMIfNotPresent
-from src.addresses import decodeAddress, addBMIfNotPresent
+from bmconfigparser import BMConfigParser
-from src.bmconfigparser import BMConfigParser
+from namecoin import namecoinConnection
-from src.namecoin import namecoinConnection
+from helper_ackPayload import genAckPayload
-from src.helper_ackPayload import genAckPayload
+from helper_generic import powQueueSize
-from src.helper_sql import sqlQuery, sqlExecute, sqlExecuteChunked, sqlStoredProcedure
+from helper_sql import sqlQuery, sqlExecute, sqlExecuteChunked, sqlStoredProcedure
-from src.helper_generic import powQueueSize
+from network.stats import pendingDownload, pendingUpload
-from src.network.stats import pendingDownload, pendingUpload
+from network.asyncore_pollchoose import set_rates
-from src.network.asyncore_pollchoose import set_rates
+from proofofwork import getPowType
+from tr import _translate
 
 
 try:
-    from src.plugins.plugin import get_plugin, get_plugins
+    from plugins.plugin import get_plugin, get_plugins
 except ImportError:
     get_plugins = False
 
@@ -1489,7 +1500,7 @@ class MyForm(settingsmixin.SMainWindow):  # pylint: disable=too-many-public-meth
         messagelist = self.getCurrentMessagelist()
         folder = self.getCurrentFolder()
         if event.key() == QtCore.Qt.Key_Delete:
-            if isinstance(focus, [MessageView, QtGui.QTableWidget]):
+            if isinstance(focus, (MessageView, QtGui.QTableWidget)):
                 if folder == "sent":
                     self.on_action_SentTrash()
                 else:
@@ -3623,7 +3634,7 @@ class MyForm(settingsmixin.SMainWindow):  # pylint: disable=too-many-public-meth
                 self.ui.tableWidgetInbox,
                 self.ui.tableWidgetInboxChans,
                 self.ui.tableWidgetInboxSubscriptions)
-        elif not isinstance(messageLists, [list, tuple]):
+        elif not isinstance(messageLists, (list, tuple)):
             messageLists = (messageLists)
         for messageList in messageLists:
             if row is not None:
@@ -4751,7 +4762,7 @@ class MyForm(settingsmixin.SMainWindow):  # pylint: disable=too-many-public-meth
     def updateStatusBar(self, data):
         """TBC"""
 
-        if isinstance(data, [tuple, list]):
+        if isinstance(data, (tuple, list)):
             option = data[1]
             message = data[0]
         else: