Merge pull request #6998 from stvstnfrd/quality/pep8

Extend PEP8 coverage

cms/urls.py

@@ -12,7 +12,8 @@ COURSELIKE_KEY_PATTERN = r'(?P<course_key_string>({}|{}))'.format(
 # Pattern to match a library key only
 LIBRARY_KEY_PATTERN = r'(?P<library_key_string>library-v1:[^/+]+\+[^/+]+)'
 
-urlpatterns = patterns('',  # nopep8
+urlpatterns = patterns(
+    '',
 
     url(r'^transcripts/upload$', 'contentstore.views.upload_transcripts', name='upload_transcripts'),
     url(r'^transcripts/download$', 'contentstore.views.download_transcripts', name='download_transcripts'),

common/djangoapps/heartbeat/urls.py

 from django.conf.urls import url, patterns
 
-urlpatterns = patterns('',  # nopep8
+urlpatterns = patterns(
+    '',
+
     url(r'^$', 'heartbeat.views.heartbeat', name='heartbeat'),
 )

common/djangoapps/pipeline_js/urls.py

@@ -3,7 +3,9 @@ URL patterns for Javascript files used to load all of the XModule JS in one wad.
 """
 from django.conf.urls import url, patterns
 
-urlpatterns = patterns('pipeline_js.views',  # nopep8
+urlpatterns = patterns(
+    'pipeline_js.views',
+
     url(r'^files\.json$', 'xmodule_js_files', name='xmodule_js_files'),
     url(r'^xmodule\.js$', 'requirejs_xmodule', name='requirejs_xmodule'),
 )

docs/en_us/enrollment_api/source/conf.py

@@ -54,8 +54,6 @@ if on_rtd:
 else:
     os.environ['DJANGO_SETTINGS_MODULE'] = 'lms'
 
-
-
 # -- General configuration -----------------------------------------------------
 
 # Add any Sphinx extension module names here, as strings. They can be extensions
@@ -72,5 +70,3 @@ exclude_patterns = ['build', 'links.rst']
 
 project = u'edX Enrollment API Version 1'
 copyright = u'2015, edX'
-
-

docs/en_us/platform_api/source/conf.py

@@ -9,12 +9,38 @@ from path import path
 import sys
 import mock
 
-MOCK_MODULES = ['lxml', 'requests', 'xblock', 'fields', 'xblock.fields',
-'frament', 'xblock.fragment', 'webob', 'multidict', 'webob.multidict', 'core',
-'xblock.core', 'runtime', 'xblock.runtime', 'sortedcontainers', 'contracts',
-'plugin', 'xblock.plugin', 'opaque_keys.edx.asides', 'asides',
-'dogstats_wrapper', 'fs', 'fs.errors', 'edxmako', 'edxmako.shortcuts',
-'shortcuts', 'crum', 'opaque_keys.edx.locator', 'LibraryLocator', 'Location']
+MOCK_MODULES = [
+    'lxml',
+    'requests',
+    'xblock',
+    'fields',
+    'xblock.fields',
+    'frament',
+    'xblock.fragment',
+    'webob',
+    'multidict',
+    'webob.multidict',
+    'core',
+    'xblock.core',
+    'runtime',
+    'xblock.runtime',
+    'sortedcontainers',
+    'contracts',
+    'plugin',
+    'xblock.plugin',
+    'opaque_keys.edx.asides',
+    'asides',
+    'dogstats_wrapper',
+    'fs',
+    'fs.errors',
+    'edxmako',
+    'edxmako.shortcuts',
+    'shortcuts',
+    'crum',
+    'opaque_keys.edx.locator',
+    'LibraryLocator',
+    'Location',
+]
 
 for mod_name in MOCK_MODULES:
     sys.modules[mod_name] = mock.Mock()
@@ -83,5 +109,3 @@ project = u'edX Platform API Version 0.5 Alpha'
 copyright = u'2015, edX'
 
 exclude_patterns = ['build', 'links.rst']
-
-

lms/djangoapps/class_dashboard/urls.py

@@ -6,7 +6,9 @@ from django.conf.urls import patterns, url
 from django.conf import settings
 COURSE_ID_PATTERN = settings.COURSE_ID_PATTERN
 
-urlpatterns = patterns('',  # nopep8
+urlpatterns = patterns(
+    '',
+
     # Json request data for metrics for entire course
     url(r'^{}/all_sequential_open_distrib$'.format(settings.COURSE_ID_PATTERN),
         'class_dashboard.views.all_sequential_open_distrib', name="all_sequential_open_distrib"),

lms/djangoapps/django_comment_client/base/urls.py

 from django.conf.urls.defaults import url, patterns
 
-urlpatterns = patterns('django_comment_client.base.views',  # nopep8
+urlpatterns = patterns(
+    'django_comment_client.base.views',
+
     url(r'upload$', 'upload', name='upload'),
     url(r'threads/(?P<thread_id>[\w\-]+)/update$', 'update_thread', name='update_thread'),
     url(r'threads/(?P<thread_id>[\w\-]+)/reply$', 'create_comment', name='create_comment'),

lms/djangoapps/django_comment_client/forum/urls.py

 from django.conf.urls.defaults import url, patterns
 
-urlpatterns = patterns('django_comment_client.forum.views',  # nopep8
+urlpatterns = patterns(
+    'django_comment_client.forum.views',
+
     url(r'users/(?P<user_id>\w+)/followed$', 'followed_threads', name='followed_threads'),
     url(r'users/(?P<user_id>\w+)$', 'user_profile', name='user_profile'),
     url(r'^(?P<discussion_id>[\w\-.]+)/threads/(?P<thread_id>\w+)$', 'single_thread', name='single_thread'),

lms/djangoapps/django_comment_client/urls.py

 from django.conf.urls.defaults import url, patterns, include
 
-urlpatterns = patterns('',  # nopep8
+urlpatterns = patterns(
+    '',
+
     url(r'forum/?', include('django_comment_client.forum.urls')),
     url(r'', include('django_comment_client.base.urls')),
 )

lms/djangoapps/instructor/views/api_urls.py

@@ -5,7 +5,9 @@ Instructor API endpoint urls.
 
 from django.conf.urls import patterns, url
 
-urlpatterns = patterns('',  # nopep8
+urlpatterns = patterns(
+    '',
+
     url(r'^students_update_enrollment$',
         'instructor.views.api.students_update_enrollment', name="students_update_enrollment"),
     url(r'^register_and_enroll_students$',

lms/djangoapps/shoppingcart/urls.py

 from django.conf.urls import patterns, url
 from django.conf import settings
 
-urlpatterns = patterns('shoppingcart.views',  # nopep8
+urlpatterns = patterns(
+    'shoppingcart.views',
+
     url(r'^postpay_callback/$', 'postpay_callback'),  # Both the ~accept and ~reject callback pages are handled here
     url(r'^receipt/(?P<ordernum>[0-9]*)/$', 'show_receipt'),
     url(r'^donation/$', 'donate', name='donation'),

lms/djangoapps/survey/urls.py

@@ -5,7 +5,9 @@ URL mappings for the Survey feature
 from django.conf.urls import patterns, url
 
 
-urlpatterns = patterns('survey.views',  # nopep8
+urlpatterns = patterns(
+    'survey.views',
+
     url(r'^(?P<survey_name>[0-9A-Za-z]+)/$', 'view_survey', name='view_survey'),
     url(r'^(?P<survey_name>[0-9A-Za-z]+)/answers/$', 'submit_answers', name='submit_answers'),
 )

lms/urls.py

@@ -12,7 +12,9 @@ if settings.DEBUG or settings.FEATURES.get('ENABLE_DJANGO_ADMIN_SITE'):
 
 # Use urlpatterns formatted as within the Django docs with first parameter "stuck" to the open parenthesis
 # pylint: disable=bad-continuation
-urlpatterns = ('',  # nopep8
+urlpatterns = (
+    '',
+
     # certificate view
     url(r'^update_certificate$', 'certificates.views.update_certificate'),
     url(r'^request_certificate$', 'certificates.views.request_certificate'),

openedx/core/djangoapps/course_groups/tests/test_cohorts.py

@@ -225,7 +225,6 @@ class TestCohorts(ModuleStoreTestCase):
         # get_cohort should return a group for user
         self.assertEquals(cohorts.get_cohort(user, course.id).name, "AutoGroup")
 
-
     def test_auto_cohorting(self):
         """
         Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups

openedx/core/djangoapps/user_api/accounts/tests/test_views.py

@@ -13,23 +13,19 @@ from student.models import UserProfile
 
 TEST_PASSWORD = "test"
 
+
 @ddt.ddt
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms')
 class TestAccountAPI(APITestCase):
 
     def setUp(self):
         super(TestAccountAPI, self).setUp()
-
         self.anonymous_client = APIClient()
-
         self.different_user = UserFactory.create(password=TEST_PASSWORD)
         self.different_client = APIClient()
-
         self.staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD)
         self.staff_client = APIClient()
-
         self.user = UserFactory.create(password=TEST_PASSWORD)
-        
         self.url = reverse("accounts_api", kwargs={'username': self.user.username})
 
     def test_get_account_anonymous_user(self):

openedx/core/djangoapps/user_api/tests/test_views.py

@@ -1599,4 +1599,3 @@ class UpdateEmailOptInTestCase(ApiTestCase, ModuleStoreTestCase):
         self.assertHttpBadRequest(response)
         with self.assertRaises(UserOrgTag.DoesNotExist):
             UserOrgTag.objects.get(user=self.user, org=self.course.id.org, key="email-optin")
-

openedx/core/operations.py

@@ -10,6 +10,7 @@ def dump_memory(signum, frame):
     """Dump memory stats for the current process to a temp directory. Uses the meliae output format."""
     scanner.dump_all_objects('{}/meliae.{}.{}.dump'.format(tempfile.gettempdir(), datetime.now().isoformat(), os.getpid()))
 
+
 def install_memory_dumper(dump_signal=signal.SIGPROF):
     """
     Install a signal handler on `signal` to dump memory stats for the current process.

pavelib/bok_choy.py

@@ -95,7 +95,8 @@ def run_bokchoy(**opts):
     msg = colorize(
         'green',
         'Running tests using {default_store} modulestore.'.format(
-            default_store=test_suite.default_store)
+            default_store=test_suite.default_store,
+        )
     )
     print(msg)
     test_suite.run()

pavelib/paver_tests/test_paver_bok_choy_cmds.py

@@ -14,15 +14,20 @@ class TestPaverBokChoyCmd(unittest.TestCase):
     def _expected_command(self, expected_text_append, expected_default_store=None):
         if expected_text_append:
             expected_text_append = "/" + expected_text_append
-        expected_statement = ("DEFAULT_STORE={default_store} SCREENSHOT_DIR='{repo_dir}/test_root/log' "
+        expected_statement = (
+            "DEFAULT_STORE={default_store} "
+            "SCREENSHOT_DIR='{repo_dir}/test_root/log' "
             "BOK_CHOY_HAR_DIR='{repo_dir}/test_root/log/hars' "
             "SELENIUM_DRIVER_LOG_DIR='{repo_dir}/test_root/log' "
             "nosetests {repo_dir}/common/test/acceptance/tests{exp_text} "
             "--with-xunit "
             "--xunit-file={repo_dir}/reports/bok_choy/xunit.xml "
-                                       "--verbosity=2 ".format(default_store=expected_default_store,
+            "--verbosity=2 "
+        ).format(
+            default_store=expected_default_store,
             repo_dir=REPO_DIR,
-                                                               exp_text=expected_text_append))
+            exp_text=expected_text_append,
+        )
         return expected_statement
 
     def test_default_bokchoy(self):

pavelib/paver_tests/test_paver_quality.py

@@ -31,9 +31,11 @@ class TestPaverQualityViolations(unittest.TestCase):
 
     @file_data('pylint_test_list.json')
     def test_pylint_parser_count_violations(self, value):
-    # Tests:
-    #     * Different types of violations
-    #     * One violation covering multiple lines
+        """
+        Tests:
+        - Different types of violations
+        - One violation covering multiple lines
+        """
         with open(self.f.name, 'w') as f:
             f.write(value)
         num = pavelib.quality._count_pylint_violations(f.name)

pavelib/quality.py

 """
 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 = {
+    '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
 @needs('pavelib.prereqs.install_python_prereqs')
@@ -17,44 +104,28 @@ def find_fixme(options):
     """
     Run pylint on system code, only looking for fixme items.
     """
-    num_fixme = 0
-    systems = getattr(options, 'system', 'lms,cms,common').split(',')
+    count = 0
+    options = _parse(options)
 
-    for system in systems:
-        # Directory to put the pylint report in.
-        # This makes the folder if it doesn't already exist.
+    for system in options['systems']:
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
-
-        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
-            )
-        )
-
+        path_list = _get_path_list(system)
+        environment_python_path = _get_python_path_prefix(system)
         sh(
-            "{pythonpath_prefix} pylint --disable R,C,W,E --enable=fixme "
-            "--msg-template={msg_template} {apps} "
+            "{environment_python_path} pylint --disable R,C,W,E --enable=fixme "
+            "--msg-template={msg_template} {path_list} "
             "| tee {report_dir}/pylint_fixme.report".format(
-                pythonpath_prefix=pythonpath_prefix,
+                environment_python_path=environment_python_path,
                 msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"',
-                apps=apps_list,
+                path_list=path_list,
                 report_dir=report_dir
             )
         )
+        count += _count_pylint_violations(
+            "{report_dir}/pylint_fixme.report".format(report_dir=report_dir)
+        )
 
-        num_fixme += _count_pylint_violations(
-            "{report_dir}/pylint_fixme.report".format(report_dir=report_dir))
-
-    print("Number of pylint fixmes: " + str(num_fixme))
+    print("Number of pylint fixmes: " + str(count))
 
 
 @task
@@ -70,52 +141,34 @@ def run_pylint(options):
     fail the task if too many violations are found.
     """
     num_violations = 0
-    violations_limit = int(getattr(options, 'limit', -1))
-    errors = getattr(options, 'errors', False)
-    systems = getattr(options, 'system', 'lms,cms,common').split(',')
+    options = _parse(options)
 
-    for system in systems:
-        # Directory to put the pylint report in.
-        # This makes the folder if it doesn't already exist.
+    for system in options['systems']:
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
 
         flags = []
-        if errors:
+        if options['errors']:
             flags.append("--errors-only")
 
-        apps = [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
-            )
-        )
-
+        path_list = _get_path_list(system)
+        environment_python_path = _get_python_path_prefix(system)
         sh(
-            "{pythonpath_prefix} pylint {flags} --msg-template={msg_template} {apps} | "
+            "{environment_python_path} pylint {flags} --msg-template={msg_template} {path_list} | "
             "tee {report_dir}/pylint.report".format(
-                pythonpath_prefix=pythonpath_prefix,
-                flags=" ".join(flags),
+                environment_python_path=environment_python_path,
+                flags=' '.join(flags),
                 msg_template='"{path}:{line}: [{msg_id}({symbol}), {obj}] {msg}"',
-                apps=apps_list,
+                path_list=path_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 > violations_limit > -1:
+    if num_violations > options['limit'] > -1:
         raise Exception("Failed. Too many pylint violations. "
-                        "The limit is {violations_limit}.".format(violations_limit=violations_limit))
+                        "The limit is {violations_limit}.".format(violations_limit=options['limit']))
 
 
 def _count_pylint_violations(report_file):
@@ -148,24 +201,19 @@ def run_pep8(options):
     Run pep8 on system code. When violations limit is passed in,
     fail the task if too many violations are found.
     """
-    num_violations = 0
-    systems = getattr(options, 'system', 'lms,cms,common').split(',')
-    violations_limit = int(getattr(options, 'limit', -1))
+    options = _parse(options)
+    count = 0
 
-    for system in systems:
-        # Directory to put the pep8 report in.
-        # This makes the folder if it doesn't already exist.
+    for system in options['systems']:
         report_dir = (Env.REPORT_DIR / system).makedirs_p()
-
         sh('pep8 {system} | tee {report_dir}/pep8.report'.format(system=system, report_dir=report_dir))
-        num_violations = num_violations + _count_pep8_violations(
-            "{report_dir}/pep8.report".format(report_dir=report_dir))
+        count += _count_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))
+    print("Number of pep8 violations: {count}".format(count=count))
+    if count:
+        raise Exception("Failed. Too many pep8 violations.")
 
 
 def _count_pep8_violations(report_file):
@@ -236,17 +284,17 @@ def run_quality(options):
     pylint_files = get_violations_reports("pylint")
     pylint_reports = u' '.join(pylint_files)
 
-    pythonpath_prefix = (
+    environment_python_path = (
         "PYTHONPATH=$PYTHONPATH:lms:lms/djangoapps:lms/lib:cms:cms/djangoapps:cms/lib:"
         "common:common/djangoapps:common/lib"
     )
 
     try:
         sh(
-            "{pythonpath_prefix} diff-quality --violations=pylint "
+            "{environment_python_path} diff-quality --violations=pylint "
             "{pylint_reports} {percentage_string} {compare_branch_string} "
             "--html-report {dquality_dir}/diff_quality_pylint.html ".format(
-                pythonpath_prefix=pythonpath_prefix,
+                environment_python_path=environment_python_path,
                 pylint_reports=pylint_reports,
                 percentage_string=percentage_string,
                 compare_branch_string=compare_branch_string,
@@ -270,10 +318,7 @@ 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.
     """
-    if "Subprocess return code: 1" not in error_message:
-        return False
-    else:
-        return True
+    return "Subprocess return code: 1" in error_message
 
 
 def get_violations_reports(violations_type):

pavelib/tests.py

@@ -204,7 +204,6 @@ def coverage(options):
                 dir=directory
             ))
 
-
     call_task('diff_coverage', options=dict(options))
 
 

pavelib/utils/test/utils.py

 """
 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

scripts/release.py

@@ -98,7 +98,7 @@ def ensure_pr_fetch():
     """
     modified = False
     remotes = git.remote().splitlines()
-    if not "edx" in remotes:
+    if 'edx' not in remotes:
         git.remote("add", "edx", "https://github.com/edx/edx-platform.git")
         modified = True
     # it would be nice to use the git-python API to do this, but it doesn't seem
@@ -251,9 +251,9 @@ def ensure_github_creds(attempts=3):
     else:
         config = {}
     # update config
-    if not "credentials" in config:
+    if 'credentials' not in config:
         config["credentials"] = {}
-    if not "api.github.com" in config["credentials"]:
+    if 'api.github.com' not in config['credentials']:
         config["credentials"]["api.github.com"] = {}
     config["credentials"]["api.github.com"]["username"] = username
     config["credentials"]["api.github.com"]["token"] = token