Follow patterns

Move init to top of class
Change asserts to more meaningful exception types
Refactor hard-to-read fn

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 = []

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'