From 79e1dae0e74648a7fdb3da4defd79e369cca1ae3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 14 Jan 2014 14:18:35 -0500 Subject: [PATCH] Fix pylint violations from #2129 --- common/djangoapps/student/roles.py | 3 --- common/lib/xmodule/xmodule/conditional_module.py | 2 +- common/lib/xmodule/xmodule/error_module.py | 2 +- common/lib/xmodule/xmodule/modulestore/__init__.py | 4 +++- .../xmodule/xmodule/modulestore/tests/test_location.py | 5 +++++ common/lib/xmodule/xmodule/modulestore/xml.py | 9 ++++++--- common/lib/xmodule/xmodule/tests/test_import.py | 4 ++++ common/lib/xmodule/xmodule/tests/test_xml_module.py | 2 -- common/lib/xmodule/xmodule/x_module.py | 2 +- common/lib/xmodule/xmodule/xml_module.py | 9 ++++----- pylintrc | 8 +++++++- 11 files changed, 32 insertions(+), 18 deletions(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 9da258e19bea..b01180198e2a 100644 --- a/common/djangoapps/student/roles.py +++ b/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)]) diff --git a/common/lib/xmodule/xmodule/conditional_module.py b/common/lib/xmodule/xmodule/conditional_module.py index 9a2332d5563f..e296af5aa5ca 100644 --- a/common/lib/xmodule/xmodule/conditional_module.py +++ b/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)) diff --git a/common/lib/xmodule/xmodule/error_module.py b/common/lib/xmodule/xmodule/error_module.py index 4f7ad25eb028..d6129299c58a 100644 --- a/common/lib/xmodule/xmodule/error_module.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/modulestore/__init__.py b/common/lib/xmodule/xmodule/modulestore/__init__.py index 0a1cf9ee106e..89b8e80d15b9 100644 --- a/common/lib/xmodule/xmodule/modulestore/__init__.py +++ b/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): ''' diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py b/common/lib/xmodule/xmodule/modulestore/tests/test_location.py index 5f4983e1bffc..f0c815f41cb0 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_location.py +++ b/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'), diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 13079bf9b3d1..664cbd980737 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/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 diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index 994603e2e11c..12f889d897b9 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -78,6 +78,7 @@ 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) @@ -85,6 +86,9 @@ class GenericXBlock(XBlock): @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.""" diff --git a/common/lib/xmodule/xmodule/tests/test_xml_module.py b/common/lib/xmodule/xmodule/tests/test_xml_module.py index 62ebf4dce0d4..d8192d7a8d6c 100644 --- a/common/lib/xmodule/xmodule/tests/test_xml_module.py +++ b/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. diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index e7bc5c47fbd8..c62d1c9a7028 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -14,7 +14,7 @@ from xblock.fields import Scope, Integer, Float, List, XBlockMixin, String from xblock.fragment import Fragment from xblock.plugin import default_select -from xblock.runtime import Runtime, MemoryIdManager +from xblock.runtime import Runtime from xmodule.fields import RelativeTime from xmodule.errortracker import exc_info_to_str diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index c659b7787bcd..d4ea3b17eee3 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/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,7 +175,7 @@ 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. @@ -184,8 +183,8 @@ def load_file(cls, filepath, fs, def_id): 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 ' % ( diff --git a/pylintrc b/pylintrc index 5a3a1ad01c29..cba312005f93 100644 --- a/pylintrc +++ b/pylintrc @@ -127,7 +127,13 @@ generated-members= build, # For xblocks fields, - +# For locations + tag, + org, + course, + category, + name, + revision, [BASIC]