Merge pull request #1137 from edx/dhm/pr271_fixups

old pr 271 comment fixups

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):
@@ -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).
     """
@@ -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 = []

common/lib/xmodule/xmodule/modulestore/split_mongo/definition_lazy_loader.py

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

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

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
 
@@ -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'
@@ -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())
 
     # ------------------------------------------------------------------

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()