From dae9b16a42d258be1d68135a88acbb4a550dc0be Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 25 Jul 2012 18:20:31 -0400 Subject: [PATCH] Clean up DescriptorSystem constructor hierarchy In response to spending an hour with a strange argument passing bug that neither I nor Cale could figure out. * pass all arguments explicitly * pass arguments in a consistent order between classes --- common/lib/xmodule/xmodule/mako_module.py | 7 +++-- .../lib/xmodule/xmodule/modulestore/mongo.py | 28 ++++++++++++++----- common/lib/xmodule/xmodule/modulestore/xml.py | 25 ++++++++--------- common/lib/xmodule/xmodule/x_module.py | 12 +++----- 4 files changed, 42 insertions(+), 30 deletions(-) diff --git a/common/lib/xmodule/xmodule/mako_module.py b/common/lib/xmodule/xmodule/mako_module.py index da620e4889..fcc47aaaaf 100644 --- a/common/lib/xmodule/xmodule/mako_module.py +++ b/common/lib/xmodule/xmodule/mako_module.py @@ -2,9 +2,12 @@ from x_module import XModuleDescriptor, DescriptorSystem class MakoDescriptorSystem(DescriptorSystem): - def __init__(self, render_template, *args, **kwargs): + def __init__(self, load_item, resources_fs, error_handler, + render_template): + super(MakoDescriptorSystem, self).__init__( + load_item, resources_fs, error_handler) + self.render_template = render_template - super(MakoDescriptorSystem, self).__init__(*args, **kwargs) class MakoModuleDescriptor(XModuleDescriptor): diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index 7cd005336c..95c882824b 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -6,6 +6,7 @@ from fs.osfs import OSFS from itertools import repeat from importlib import import_module +from xmodule.errorhandlers import strict_error_handler from xmodule.x_module import XModuleDescriptor from xmodule.mako_module import MakoDescriptorSystem from mitxmako.shortcuts import render_to_string @@ -23,15 +24,26 @@ class CachingDescriptorSystem(MakoDescriptorSystem): A system that has a cache of module json that it will use to load modules from, with a backup of calling to the underlying modulestore for more data """ - def __init__(self, modulestore, module_data, default_class, resources_fs, render_template): + def __init__(self, modulestore, module_data, default_class, resources_fs, + error_handler, render_template): """ modulestore: the module store that can be used to retrieve additional modules - module_data: a dict mapping Location -> json that was cached from the underlying modulestore - default_class: The default_class to use when loading an XModuleDescriptor from the module_data + + module_data: a dict mapping Location -> json that was cached from the + underlying modulestore + + default_class: The default_class to use when loading an + XModuleDescriptor from the module_data + resources_fs: a filesystem, as per MakoDescriptorSystem - render_template: a function for rendering templates, as per MakoDescriptorSystem + + error_handler: + + render_template: a function for rendering templates, as per + MakoDescriptorSystem """ - super(CachingDescriptorSystem, self).__init__(render_template, self.load_item, resources_fs) + super(CachingDescriptorSystem, self).__init__( + self.load_item, resources_fs, error_handler, render_template) self.modulestore = modulestore self.module_data = module_data self.default_class = default_class @@ -127,13 +139,15 @@ class MongoModuleStore(ModuleStore): """ Load an XModuleDescriptor from item, using the children stored in data_cache """ - resource_fs = OSFS(self.fs_root / item.get('data_dir', item['location']['course'])) + resource_fs = OSFS(self.fs_root / item.get('data_dir', + item['location']['course'])) system = CachingDescriptorSystem( self, data_cache, self.default_class, resource_fs, - render_to_string + strict_error_handler, + render_to_string, ) return system.load_item(item['location']) diff --git a/common/lib/xmodule/xmodule/modulestore/xml.py b/common/lib/xmodule/xmodule/modulestore/xml.py index 8965f3d028..23b812baae 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml.py +++ b/common/lib/xmodule/xmodule/modulestore/xml.py @@ -13,8 +13,9 @@ import re from . import ModuleStore, Location from .exceptions import ItemNotFoundError -etree.set_default_parser(etree.XMLParser(dtd_validation=False, load_dtd=False, - remove_comments=True, remove_blank_text=True)) +etree.set_default_parser( + etree.XMLParser(dtd_validation=False, load_dtd=False, + remove_comments=True, remove_blank_text=True)) log = logging.getLogger('mitx.' + __name__) @@ -28,8 +29,7 @@ def clean_out_mako_templating(xml_string): return xml_string class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): - def __init__(self, xmlstore, org, course, course_dir, - error_handler): + def __init__(self, xmlstore, org, course, course_dir, error_handler): """ A class that handles loading from xml. Does some munging to ensure that all elements have unique slugs. @@ -82,15 +82,14 @@ class ImportSystem(XMLParsingSystem, MakoDescriptorSystem): module.get_children() return module - system_kwargs = dict( - render_template=lambda: '', - load_item=xmlstore.get_item, - resources_fs=OSFS(xmlstore.data_dir / course_dir), - process_xml=process_xml, - error_handler=error_handler, - ) - MakoDescriptorSystem.__init__(self, **system_kwargs) - XMLParsingSystem.__init__(self, **system_kwargs) + render_template = lambda: '' + load_item = xmlstore.get_item + resources_fs = OSFS(xmlstore.data_dir / course_dir) + + MakoDescriptorSystem.__init__(self, load_item, resources_fs, + error_handler, render_template) + XMLParsingSystem.__init__(self, load_item, resources_fs, + error_handler, process_xml) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 12881a0434..caeabb5ff5 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -521,9 +521,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet): class DescriptorSystem(object): - def __init__(self, load_item, resources_fs, - error_handler, - **kwargs): + def __init__(self, load_item, resources_fs, error_handler): """ load_item: Takes a Location and returns an XModuleDescriptor @@ -563,16 +561,14 @@ class DescriptorSystem(object): class XMLParsingSystem(DescriptorSystem): - def __init__(self, load_item, resources_fs, process_xml, **kwargs): + def __init__(self, load_item, resources_fs, error_handler, process_xml): """ - load_item: Takes a Location and returns an XModuleDescriptor + load_item, resources_fs, error_handler: see DescriptorSystem process_xml: Takes an xml string, and returns a XModuleDescriptor created from that xml - - """ - DescriptorSystem.__init__(self, load_item, resources_fs, **kwargs) + DescriptorSystem.__init__(self, load_item, resources_fs, error_handler) self.process_xml = process_xml