From f8c727847067fe53d79c0a45c0019c1045c670e1 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 26 Sep 2013 11:25:10 -0400 Subject: [PATCH 1/2] Follow patterns Move init to top of class Change asserts to more meaningful exception types Refactor hard-to-read fn --- .../xmodule/xmodule/modulestore/locator.py | 98 +++++++++---------- .../modulestore/tests/test_locators.py | 6 +- 2 files changed, 52 insertions(+), 52 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index dfeafa5e1d..097f69c7c2 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -114,6 +114,30 @@ class CourseLocator(Locator): course_id = None branch = None + def __init__(self, url=None, version_guid=None, course_id=None, branch=None): + """ + Construct a CourseLocator + Caller may provide url (but no other parameters). + Caller may provide version_guid (but no other parameters). + Caller may provide course_id (optionally provide branch). + + Resulting CourseLocator will have either a version_guid property + or a course_id (with optional branch) property, or both. + + version_guid must be an instance of bson.objectid.ObjectId or None + url, course_id, and branch must be strings or None + + """ + self._validate_args(url, version_guid, course_id) + if url: + self.init_from_url(url) + if version_guid: + self.init_from_version_guid(version_guid) + if course_id or branch: + self.init_from_course_id(course_id, branch) + assert self.version_guid or self.course_id, \ + "Either version_guid or course_id should be set." + def __unicode__(self): """ Return a string representing this location. @@ -135,18 +159,13 @@ class CourseLocator(Locator): """ return 'edx://' + unicode(self) - # -- unused args which are used via inspect - # pylint: disable= W0613 - def validate_args(self, url, version_guid, course_id, branch): + def _validate_args(self, url, version_guid, course_id): """ - Validate provided arguments. + Validate provided arguments. Internal use only which is why it checks for each + arg and doesn't use keyword """ - need_oneof = set(('url', 'version_guid', 'course_id')) - args, _, _, values = inspect.getargvalues(inspect.currentframe()) - provided_args = [a for a in args if a != 'self' and values[a] is not None] - if len(need_oneof.intersection(provided_args)) == 0: - raise InsufficientSpecificationError("Must provide one of these args: %s " % - list(need_oneof)) + if not any((url, version_guid, course_id)): + raise InsufficientSpecificationError("Must provide one of url, version_guid, course_id") def is_fully_specified(self): """ @@ -154,8 +173,8 @@ class CourseLocator(Locator): are specified. This should always return True, since this should be validated in the constructor. """ - return self.version_guid is not None \ - or (self.course_id is not None and self.branch is not None) + return (self.version_guid is not None or + (self.course_id is not None and self.branch is not None)) def set_course_id(self, new): """ @@ -189,30 +208,6 @@ class CourseLocator(Locator): version_guid=self.version_guid, branch=self.branch) - def __init__(self, url=None, version_guid=None, course_id=None, branch=None): - """ - Construct a CourseLocator - Caller may provide url (but no other parameters). - Caller may provide version_guid (but no other parameters). - Caller may provide course_id (optionally provide branch). - - Resulting CourseLocator will have either a version_guid property - or a course_id (with optional branch) property, or both. - - version_guid must be an instance of bson.objectid.ObjectId or None - url, course_id, and branch must be strings or None - - """ - self.validate_args(url, version_guid, course_id, branch) - if url: - self.init_from_url(url) - if version_guid: - self.init_from_version_guid(version_guid) - if course_id or branch: - self.init_from_course_id(course_id, branch) - assert self.version_guid or self.course_id, \ - "Either version_guid or course_id should be set." - @classmethod def as_object_id(cls, value): """ @@ -233,9 +228,11 @@ class CourseLocator(Locator): """ if isinstance(url, Locator): url = url.url() - assert isinstance(url, basestring), '%s is not an instance of basestring' % url + if not isinstance(url, basestring): + raise TypeError('%s is not an instance of basestring' % url) parse = parse_url(url) - assert parse, 'Could not parse "%s" as a url' % url + if not parse: + raise ValueError('Could not parse "%s" as a url' % url) self._set_value( parse, 'version_guid', lambda (new_guid): self.set_version_guid(self.as_object_id(new_guid)) ) @@ -250,13 +247,13 @@ class CourseLocator(Locator): """ version_guid = self.as_object_id(version_guid) - assert isinstance(version_guid, ObjectId), \ - '%s is not an instance of ObjectId' % version_guid + if not isinstance(version_guid, ObjectId): + raise TypeError('%s is not an instance of ObjectId' % version_guid) self.set_version_guid(version_guid) def init_from_course_id(self, course_id, explicit_branch=None): """ - Course_id is a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. + Course_id is a CourseLocator or a string like 'mit.eecs.6002x' or 'mit.eecs.6002x/branch/published'. Revision (optional) is a string like 'published'. It may be provided explicitly (explicit_branch) or embedded into course_id. @@ -270,10 +267,12 @@ class CourseLocator(Locator): if course_id: if isinstance(course_id, CourseLocator): course_id = course_id.course_id - assert course_id, "%s does not have a valid course_id" + if not course_id: + raise ValueError("%s does not have a valid course_id" % course_id) parse = parse_course_id(course_id) - assert parse, 'Could not parse "%s" as a course_id' % course_id + if not parse: + raise ValueError('Could not parse "%s" as a course_id' % course_id) self.set_course_id(parse['id']) rev = parse['branch'] if rev: @@ -348,7 +347,7 @@ class BlockUsageLocator(CourseLocator): url, course_id, branch, and usage_id must be strings or None """ - self.validate_args(url, version_guid, course_id, branch) + self._validate_args(url, version_guid, course_id) if url: self.init_block_ref_from_url(url) if course_id: @@ -398,7 +397,8 @@ class BlockUsageLocator(CourseLocator): self.set_usage_id(block_ref) else: parse = parse_block_ref(block_ref) - assert parse, 'Could not parse "%s" as a block_ref' % block_ref + if not parse: + raise ValueError('Could not parse "%s" as a block_ref' % block_ref) self.set_usage_id(parse['block']) def init_block_ref_from_url(self, url): @@ -461,10 +461,10 @@ class VersionTree(object): """ :param locator: must be version specific (Course has version_guid or definition had id) """ - assert isinstance(locator, Locator) and not inspect.isabstract(locator), \ - "locator must be a concrete subclass of Locator" - assert locator.version(), \ - "locator must be version specific (Course has version_guid or definition had id)" + if not isinstance(locator, Locator) and not inspect.isabstract(locator): + raise TypeError("locator {} must be a concrete subclass of Locator".format(locator)) + if not locator.version(): + raise ValueError("locator must be version specific (Course has version_guid or definition had id)") self.locator = locator if tree_dict is None: self.children = [] diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 05f4d25283..330a2b7320 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -91,8 +91,8 @@ class LocatorTest(TestCase): 'mit.eecs' + BRANCH_PREFIX + 'this ', 'mit.eecs' + BRANCH_PREFIX + 'th%is ', ): - self.assertRaises(AssertionError, CourseLocator, course_id=bad_id) - self.assertRaises(AssertionError, CourseLocator, url='edx://' + bad_id) + self.assertRaises(ValueError, CourseLocator, course_id=bad_id) + self.assertRaises(ValueError, CourseLocator, url='edx://' + bad_id) def test_course_constructor_bad_url(self): for bad_url in ('edx://', @@ -100,7 +100,7 @@ class LocatorTest(TestCase): 'http://mit.eecs', 'mit.eecs', 'edx//mit.eecs'): - self.assertRaises(AssertionError, CourseLocator, url=bad_url) + self.assertRaises(ValueError, CourseLocator, url=bad_url) def test_course_constructor_redundant_001(self): testurn = 'mit.eecs.6002x' From d79220122c2b5c743a49e21e60d76677f2b5e605 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Thu, 26 Sep 2013 11:27:29 -0400 Subject: [PATCH 2/2] Change DescriptionLocator to DefinitionLocator --- common/lib/xmodule/xmodule/modulestore/locator.py | 2 +- .../modulestore/split_mongo/definition_lazy_loader.py | 4 ++-- .../lib/xmodule/xmodule/modulestore/split_mongo/split.py | 8 ++++---- .../xmodule/xmodule/modulestore/tests/test_locators.py | 6 +++--- .../xmodule/modulestore/tests/test_split_modulestore.py | 8 ++++---- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/locator.py b/common/lib/xmodule/xmodule/modulestore/locator.py index 097f69c7c2..47753a4941 100644 --- a/common/lib/xmodule/xmodule/modulestore/locator.py +++ b/common/lib/xmodule/xmodule/modulestore/locator.py @@ -424,7 +424,7 @@ class BlockUsageLocator(CourseLocator): return rep + BLOCK_PREFIX + unicode(self.usage_id) -class DescriptionLocator(Locator): +class DefinitionLocator(Locator): """ Container for how to locate a description (the course-independent content). """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py index 5ccaaa7ed3..9b2a652a95 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py @@ -1,4 +1,4 @@ -from xmodule.modulestore.locator import DescriptionLocator +from xmodule.modulestore.locator import DefinitionLocator class DefinitionLazyLoader(object): @@ -15,7 +15,7 @@ class DefinitionLazyLoader(object): :param definition_locator: the id of the record in the above to fetch """ self.modulestore = modulestore - self.definition_locator = DescriptionLocator(definition_id) + self.definition_locator = DefinitionLocator(definition_id) def fetch(self): """ diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 60bfa66b6a..cdcb55d899 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -11,7 +11,7 @@ from pytz import UTC from xmodule.errortracker import null_error_tracker from xmodule.x_module import XModuleDescriptor -from xmodule.modulestore.locator import BlockUsageLocator, DescriptionLocator, CourseLocator, VersionTree, LocalId +from xmodule.modulestore.locator import BlockUsageLocator, DefinitionLocator, CourseLocator, VersionTree, LocalId from xmodule.modulestore.exceptions import InsufficientSpecificationError, VersionConflictError, DuplicateItemError from xmodule.modulestore import inheritance, ModuleStoreBase, Location @@ -563,7 +563,7 @@ class SplitMongoModuleStore(ModuleStoreBase): } } new_id = self.definitions.insert(document) - definition_locator = DescriptionLocator(new_id) + definition_locator = DefinitionLocator(new_id) document['edit_info']['original_version'] = new_id self.definitions.update({'_id': new_id}, {'$set': {"edit_info.original_version": new_id}}) return definition_locator @@ -597,7 +597,7 @@ class SplitMongoModuleStore(ModuleStoreBase): old_definition['edit_info']['edited_on'] = datetime.datetime.now(UTC) old_definition['edit_info']['previous_version'] = definition_locator.definition_id new_id = self.definitions.insert(old_definition) - return DescriptionLocator(new_id), True + return DefinitionLocator(new_id), True else: return definition_locator, False @@ -1252,7 +1252,7 @@ class SplitMongoModuleStore(ModuleStoreBase): elif '_id' not in definition: return None else: - return DescriptionLocator(definition['_id']) + return DefinitionLocator(definition['_id']) def internal_clean_children(self, course_locator): """ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py index 330a2b7320..6ec9acfcd5 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_locators.py @@ -4,7 +4,7 @@ Tests for xmodule.modulestore.locator. from unittest import TestCase from bson.objectid import ObjectId -from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DescriptionLocator +from xmodule.modulestore.locator import Locator, CourseLocator, BlockUsageLocator, DefinitionLocator from xmodule.modulestore.parsers import BRANCH_PREFIX, BLOCK_PREFIX, VERSION_PREFIX, URL_VERSION_PREFIX from xmodule.modulestore.exceptions import InsufficientSpecificationError, OverSpecificationError @@ -254,11 +254,11 @@ class LocatorTest(TestCase): self.assertEqual('BlockUsageLocator("mit.eecs.6002x/branch/published/block/HW3")', repr(testobj)) def test_description_locator_url(self): - definition_locator = DescriptionLocator("chapter12345_2") + definition_locator = DefinitionLocator("chapter12345_2") self.assertEqual('edx://' + URL_VERSION_PREFIX + 'chapter12345_2', definition_locator.url()) def test_description_locator_version(self): - definition_locator = DescriptionLocator("chapter12345_2") + definition_locator = DefinitionLocator("chapter12345_2") self.assertEqual("chapter12345_2", definition_locator.version()) # ------------------------------------------------------------------ diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py index be5c61a143..2373c409de 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py @@ -13,7 +13,7 @@ from xblock.fields import Scope from xmodule.course_module import CourseDescriptor from xmodule.modulestore.exceptions import InsufficientSpecificationError, ItemNotFoundError, VersionConflictError, \ DuplicateItemError -from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DescriptionLocator +from xmodule.modulestore.locator import CourseLocator, BlockUsageLocator, VersionTree, DefinitionLocator from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.x_module import XModuleMixin from pytz import UTC @@ -562,7 +562,7 @@ class TestItemCrud(SplitModuleTest): new_module = modulestore().create_item( locator, category, 'user123', fields={'display_name': 'new chapter'}, - definition_locator=DescriptionLocator("chapter12345_2") + definition_locator=DefinitionLocator("chapter12345_2") ) # check that course version changed and course's previous is the other one self.assertNotEqual(new_module.location.version_guid, premod_course.location.version_guid) @@ -588,7 +588,7 @@ class TestItemCrud(SplitModuleTest): another_module = modulestore().create_item( locator, category, 'anotheruser', fields={'display_name': 'problem 2', 'data': another_payload}, - definition_locator=DescriptionLocator("problem12345_3_1"), + definition_locator=DefinitionLocator("problem12345_3_1"), ) # check that course version changed and course's previous is the other one parent = modulestore().get_item(locator) @@ -789,7 +789,7 @@ class TestItemCrud(SplitModuleTest): modulestore().create_item( locator, category, 'test_update_manifold', fields={'display_name': 'problem 2', 'data': another_payload}, - definition_locator=DescriptionLocator("problem12345_3_1"), + definition_locator=DefinitionLocator("problem12345_3_1"), ) # pylint: disable=W0212 modulestore()._clear_cache()