Merge pull request #7187 from edx/benp/revert-partof-6998

Revert "Refactor quality checks"

pavelib/quality.py

@@ -1,98 +1,13 @@
 """
 Check code quality using pep8, pylint, and diff_quality.
 """
-from __future__ import print_function
 from paver.easy import sh, task, cmdopts, needs, BuildFailure
 import os
 import re
 
 from .utils.envs import Env
 
-DIRECTORIES_TOP_LEVEL_COMMON = {
+ALL_SYSTEMS = 'lms,cms,common'
-    'common',
-    'openedx',
-}
-
-DIRECTORIES_TOP_LEVEL_SYSTEMS = {
-    'cms',
-    'docs',
-    'lms',
-    'pavelib',
-    'scripts',
-}
-
-DIRECTORIES_TOP_LEVEL_ALL = set()
-DIRECTORIES_TOP_LEVEL_ALL.update(DIRECTORIES_TOP_LEVEL_COMMON)
-DIRECTORIES_TOP_LEVEL_ALL.update(DIRECTORIES_TOP_LEVEL_SYSTEMS)
-
-DIRECTORIES_INNER = {
-    'djangoapps',
-    'lib',
-}
-
-
-def _get_path_list(system):
-    """
-    Gather a list of subdirectories within the system
-
-    :param system: the system directory to search; e.g. 'lms', 'cms'
-    :returns: a list of system subdirectories to be linted
-    """
-    paths = [
-        system,
-    ]
-
-    for directory in DIRECTORIES_INNER:
-        try:
-            directories = os.listdir(os.path.join(system, directory))
-        except OSError:
-            pass
-        else:
-            paths.extend([
-                directory
-                for directory in directories
-                if os.path.isdir(os.path.join(system, directory, directory))
-            ])
-
-    path_list = ' '.join(paths)
-    return path_list
-
-
-def _get_python_path_prefix(directory_system):
-    """
-    Build a string to specify the PYTHONPATH environment variable
-
-    :param directory_system: the system directory to search; e.g. 'lms', 'cms'
-    :returns str: a PYTHONPATH environment string for the command line
-    """
-    paths = {
-        directory_system,
-    }
-    directories_all = set(DIRECTORIES_TOP_LEVEL_COMMON)
-    directories_all.add(directory_system)
-    for system in directories_all:
-        for subsystem in DIRECTORIES_INNER:
-            path = os.path.join(system, subsystem)
-            paths.add(path)
-    paths = ':'.join(paths)
-    environment_python_path = "PYTHONPATH={paths}".format(
-        paths=paths,
-    )
-    return environment_python_path
-
-
-def _parse(options):
-    """
-    Parse command line options, setting sane defaults
-
-    :param options: a Paver Options object
-    :returns dict: containing: errors, system, limit
-    """
-    return {
-        'errors': getattr(options, 'errors', False),
-        'systems': getattr(options, 'system', ','.join(DIRECTORIES_TOP_LEVEL_ALL)).split(','),
-        'limit': int(getattr(options, 'limit', -1)),
-    }
 
 
 @task
@@ -104,28 +19,44 @@ def find_fixme(options):
     """
     Run pylint on system code, only looking for fixme items.
     """
-    count = 0
+    num_fixme = 0
-    options = _parse(options)
+    systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
 
-    for system in options['systems']:
+    for system in systems:
+        # Directory to put the pylint report in.
+        # This makes the folder if it doesn't already exist.
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
-        path_list = _get_path_list(system)
+
-        environment_python_path = _get_python_path_prefix(system)
+        apps = [system]
+
+        for directory in ['djangoapps', 'lib']:
+            dirs = os.listdir(os.path.join(system, directory))
+            apps.extend([d for d in dirs if os.path.isdir(os.path.join(system, directory, d))])
+
+        apps_list = ' '.join(apps)
+
+        pythonpath_prefix = (
+            "PYTHONPATH={system}:{system}/lib"
+            "common/djangoapps:common/lib".format(
+                system=system
+            )
+        )
+
         sh(
-            "{environment_python_path} pylint --disable R,C,W,E --enable=fixme "
+            "{pythonpath_prefix} pylint --disable R,C,W,E --enable=fixme "
-            "--msg-template={msg_template} {path_list} "
+            "--msg-template={msg_template} {apps} "
             "| tee {report_dir}/pylint_fixme.report".format(
-                environment_python_path=environment_python_path,
+                pythonpath_prefix=pythonpath_prefix,
                 msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"',
-                path_list=path_list,
+                apps=apps_list,
                 report_dir=report_dir
             )
         )
-        count += _count_pylint_violations(
-            "{report_dir}/pylint_fixme.report".format(report_dir=report_dir)
-        )
 
-    print("Number of pylint fixmes: " + str(count))
+        num_fixme += _count_pylint_violations(
+            "{report_dir}/pylint_fixme.report".format(report_dir=report_dir))
+
+    print("Number of pylint fixmes: " + str(num_fixme))
 
 
 @task
@@ -141,34 +72,52 @@ def run_pylint(options):
     fail the task if too many violations are found.
     """
     num_violations = 0
-    options = _parse(options)
+    violations_limit = int(getattr(options, 'limit', -1))
+    errors = getattr(options, 'errors', False)
+    systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
 
-    for system in options['systems']:
+    for system in systems:
+        # Directory to put the pylint report in.
+        # This makes the folder if it doesn't already exist.
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
 
         flags = []
-        if options['errors']:
+        if errors:
             flags.append("--errors-only")
 
-        path_list = _get_path_list(system)
+        apps = [system]
-        environment_python_path = _get_python_path_prefix(system)
+
+        for directory in ['lib']:
+            dirs = os.listdir(os.path.join(system, directory))
+            apps.extend([d for d in dirs if os.path.isdir(os.path.join(system, directory, d))])
+
+        apps_list = ' '.join(apps)
+
+        pythonpath_prefix = (
+            "PYTHONPATH={system}:{system}/djangoapps:{system}/"
+            "lib:common/djangoapps:common/lib".format(
+                system=system
+            )
+        )
+
         sh(
-            "{environment_python_path} pylint {flags} --msg-template={msg_template} {path_list} | "
+            "{pythonpath_prefix} pylint {flags} --msg-template={msg_template} {apps} | "
             "tee {report_dir}/pylint.report".format(
-                environment_python_path=environment_python_path,
+                pythonpath_prefix=pythonpath_prefix,
-                flags=' '.join(flags),
+                flags=" ".join(flags),
                 msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"',
-                path_list=path_list,
+                apps=apps_list,
                 report_dir=report_dir
             )
         )
+
         num_violations += _count_pylint_violations(
             "{report_dir}/pylint.report".format(report_dir=report_dir))
 
     print("Number of pylint violations: " + str(num_violations))
-    if num_violations > options['limit'] > -1:
+    if num_violations > violations_limit > -1:
         raise Exception("Failed. Too many pylint violations. "
-                        "The limit is {violations_limit}.".format(violations_limit=options['limit']))
+                        "The limit is {violations_limit}.".format(violations_limit=violations_limit))
 
 
 def _count_pylint_violations(report_file):
@@ -201,19 +150,24 @@ def run_pep8(options):
     Run pep8 on system code. When violations limit is passed in,
     fail the task if too many violations are found.
     """
-    options = _parse(options)
+    num_violations = 0
-    count = 0
+    systems = getattr(options, 'system', ALL_SYSTEMS).split(',')
+    violations_limit = int(getattr(options, 'limit', -1))
 
-    for system in options['systems']:
+    for system in systems:
+        # Directory to put the pep8 report in.
+        # This makes the folder if it doesn't already exist.
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
-        sh('pep8 {system} | tee {report_dir}/pep8.report'.format(system=system, report_dir=report_dir))
-        count += _count_pep8_violations(
-            "{report_dir}/pep8.report".format(report_dir=report_dir)
-        )
 
-    print("Number of pep8 violations: {count}".format(count=count))
+        sh('pep8 {system} | tee {report_dir}/pep8.report'.format(system=system, report_dir=report_dir))
-    if count:
+        num_violations = num_violations + _count_pep8_violations(
-        raise Exception("Failed. Too many pep8 violations.")
+            "{report_dir}/pep8.report".format(report_dir=report_dir))
+
+    print("Number of pep8 violations: " + str(num_violations))
+    # Fail the task if the violations limit has been reached
+    if num_violations > violations_limit > -1:
+        raise Exception("Failed. Too many pep8 violations. "
+                        "The limit is {violations_limit}.".format(violations_limit=violations_limit))
 
 
 def _count_pep8_violations(report_file):
@@ -284,17 +238,17 @@ def run_quality(options):
     pylint_files = get_violations_reports("pylint")
     pylint_reports = u' '.join(pylint_files)
 
-    environment_python_path = (
+    pythonpath_prefix = (
         "PYTHONPATH=$PYTHONPATH:lms:lms/djangoapps:lms/lib:cms:cms/djangoapps:cms/lib:"
         "common:common/djangoapps:common/lib"
     )
 
     try:
         sh(
-            "{environment_python_path} diff-quality --violations=pylint "
+            "{pythonpath_prefix} diff-quality --violations=pylint "
             "{pylint_reports} {percentage_string} {compare_branch_string} "
             "--html-report {dquality_dir}/diff_quality_pylint.html ".format(
-                environment_python_path=environment_python_path,
+                pythonpath_prefix=pythonpath_prefix,
                 pylint_reports=pylint_reports,
                 percentage_string=percentage_string,
                 compare_branch_string=compare_branch_string,
@@ -318,7 +272,10 @@ def is_percentage_failure(error_message):
     paver with a subprocess return code error. If the subprocess exits with anything other than 1, raise
     a paver exception.
     """
-    return "Subprocess return code: 1" in error_message
+    if "Subprocess return code: 1" not in error_message:
+        return False
+    else:
+        return True
 
 
 def get_violations_reports(violations_type):

pavelib/utils/test/utils.py

@@ -1,7 +1,6 @@
 """
 Helper functions for test tasks
 """
-from __future__ import print_function
 from paver.easy import sh, task, cmdopts
 from pavelib.utils.envs import Env
 import os