Merge pull request #2171 from cpennington/pylint-cleanup

Fix pylint violations from #2129

common/djangoapps/student/roles.py

@@ -157,7 +157,6 @@ def __init__(self, role, location, course_context=None):
         # direct copy from auth.authz.get_all_course_role_groupnames will refactor to one impl asap
         groupnames = []
 
-        # pylint: disable=no-member
         if isinstance(self.location, Location):
             try:
                 groupnames.append('{0}_{1}'.format(role, self.location.course_id))
@@ -193,8 +192,6 @@ class OrgRole(GroupBasedRole):
     A named role in a particular org
     """
     def __init__(self, role, location):
-        # pylint: disable=no-member
-
         location = Location(location)
         super(OrgRole, self).__init__(['{}_{}'.format(role, location.org)])
 

common/lib/xmodule/xmodule/conditional_module.py

@@ -224,7 +224,7 @@ def definition_from_xml(cls, xml_object, system):
             if child.tag == 'show':
                 location = ConditionalDescriptor.parse_sources(child, system)
                 children.extend(location)
-                show_tag_list.extend(location.url())
+                show_tag_list.extend(location.url())  # pylint: disable=no-member
             else:
                 try:
                     descriptor = system.process_xml(etree.tostring(child))

common/lib/xmodule/xmodule/error_module.py

@@ -134,7 +134,7 @@ def from_descriptor(cls, descriptor, error_msg='Error not available'):
         )
 
     @classmethod
-    def from_xml(cls, xml_data, system, id_generator,
+    def from_xml(cls, xml_data, system, id_generator,  # pylint: disable=arguments-differ
                  error_msg='Error not available'):
         '''Create an instance of this descriptor from the supplied data.
 

common/lib/xmodule/xmodule/modulestore/__init__.py

@@ -268,7 +268,9 @@ def _replace(self, **kwargs):
                 _check_location_part(value, INVALID_CHARS_NAME)
             else:
                 _check_location_part(value, INVALID_CHARS)
-        return super(Location, self)._replace(**kwargs)
+
+        # namedtuple is an old-style class, so don't use super
+        return _LocationBase._replace(self, **kwargs)
 
     def replace(self, **kwargs):
         '''

common/lib/xmodule/xmodule/modulestore/tests/test_location.py

@@ -19,6 +19,9 @@
 
 @ddt.ddt
 class TestLocations(TestCase):
+    """
+    Tests of :class:`.Location`
+    """
     @ddt.data(
         "tag://org/course/category/name",
         "tag://org/course/category/name@revision"
@@ -173,6 +176,8 @@ def test_course_id(self):
             loc.course_id  # pylint: disable=pointless-statement
 
     def test_replacement(self):
+        # pylint: disable=protected-access
+
         self.assertEquals(
             Location('t://o/c/c/n@r')._replace(name='new_name'),
             Location('t://o/c/c/new_name@r'),

common/lib/xmodule/xmodule/modulestore/xml.py

@@ -18,10 +18,9 @@
 from xmodule.errortracker import make_error_tracker, exc_info_to_str
 from xmodule.course_module import CourseDescriptor
 from xmodule.mako_module import MakoDescriptorSystem
-from xmodule.x_module import XMLParsingSystem, prefer_xmodules, policy_key
+from xmodule.x_module import XMLParsingSystem, policy_key
 
 from xmodule.html_module import HtmlDescriptor
-from xblock.core import XBlock
 from xblock.fields import ScopeIds
 from xblock.field_data import DictFieldData
 from xblock.runtime import DictKeyValueStore, IdReader, IdGenerator
@@ -222,6 +221,7 @@ def fallback_name(orig_name=None):
         # load_item should actually be get_instance, because it expects the course-specific
         # policy to be loaded.  For now, just add the course_id here...
         def load_item(location):
+            """Return the XBlock for the specified location"""
             return xmlstore.get_instance(course_id, Location(location))
 
         resources_fs = OSFS(xmlstore.data_dir / course_dir)
@@ -509,6 +509,9 @@ def load_course(self, course_dir, tracker):
             course_id = CourseDescriptor.make_id(org, course, url_name)
 
             def get_policy(usage_id):
+                """
+                Return the policy dictionary to be applied to the specified XBlock usage
+                """
                 return policy.get(policy_key(usage_id), {})
 
             system = ImportSystem(
@@ -570,7 +573,7 @@ def _load_extra_content(self, system, course_descriptor, category, path, course_
                     html = f.read().decode('utf-8')
                     # tabs are referenced in policy.json through a 'slug' which is just the filename without the .html suffix
                     slug = os.path.splitext(os.path.basename(filepath))[0]
-                    loc = course_descriptor.scope_ids.usage_id._replace(category=category, name=slug)
+                    loc = course_descriptor.scope_ids.usage_id.replace(category=category, name=slug)
                     module = system.construct_xblock_from_class(
                         HtmlDescriptor,
                         # We're loading a descriptor, so student_id is meaningless

common/lib/xmodule/xmodule/tests/test_import.py

@@ -78,13 +78,17 @@ def get_course(self, name):
 
 
 class GenericXBlock(XBlock):
+    """XBlock for testing pure xblock xml import"""
     has_children = True
     field1 = String(default="something", scope=Scope.user_state)
     field2 = Integer(scope=Scope.user_state)
 
 
 @ddt.ddt
 class PureXBlockImportTest(BaseCourseTestCase):
+    """
+    Tests of import pure XBlocks (not XModules) from xml
+    """
 
     def assert_xblocks_are_good(self, block):
         """Assert a number of conditions that must be true for `block` to be good."""

common/lib/xmodule/xmodule/tests/test_xml_module.py

@@ -119,13 +119,11 @@ def test_inherited_across_generations(self):
         parent = self.get_a_block(usage_id="parent")
         parent.inherited = "Changed!"
         self.assertEqual(parent.inherited, "Changed!")
-        parent_id = "parent"
         for child_num in range(10):
             usage_id = "child_{}".format(child_num)
             child = self.get_a_block(usage_id=usage_id)
             child.parent = "parent"
             self.assertEqual(child.inherited, "Changed!")
-            parent_id = usage_id
 
     def test_not_inherited(self):
         # Fields not in the inherited_names list won't be inherited.

common/lib/xmodule/xmodule/xml_module.py

@@ -6,8 +6,7 @@
 from lxml import etree
 
 from xblock.fields import Dict, Scope, ScopeIds
-from xmodule.x_module import XModuleDescriptor, policy_key
-from xmodule.modulestore import Location
+from xmodule.x_module import XModuleDescriptor
 from xmodule.modulestore.inheritance import own_metadata, InheritanceKeyValueStore
 from xmodule.modulestore.xml_exporter import EdxJSONEncoder
 from xblock.runtime import KvsFieldData
@@ -176,16 +175,16 @@ def file_to_xml(cls, file_object):
         return etree.parse(file_object, parser=edx_xml_parser).getroot()
 
     @classmethod
-    def load_file(cls, filepath, fs, def_id):
+    def load_file(cls, filepath, fs, def_id):  # pylint: disable=invalid-name
         '''
         Open the specified file in fs, and call cls.file_to_xml on it,
         returning the lxml object.
 
         Add details and reraise on error.
         '''
         try:
-            with fs.open(filepath) as file:
-                return cls.file_to_xml(file)
+            with fs.open(filepath) as xml_file:
+                return cls.file_to_xml(xml_file)
         except Exception as err:
             # Add info about where we are, but keep the traceback
             msg = 'Unable to load file contents at path %s for item %s: %s ' % (

pylintrc

@@ -127,7 +127,13 @@ generated-members=
     build,
 # For xblocks
     fields,
-
+# For locations
+    tag,
+    org,
+    course,
+    category,
+    name,
+    revision,
 
 [BASIC]