From f57414b44f6ff490697dcb59ceb7e68642b66ee0 Mon Sep 17 00:00:00 2001 From: cewing Date: Thu, 14 May 2015 15:48:27 -0700 Subject: [PATCH 01/25] coverage should be executable --- scripts/coverage | 0 1 file changed, 0 insertions(+), 0 deletions(-) mode change 100644 => 100755 scripts/coverage diff --git a/scripts/coverage b/scripts/coverage old mode 100644 new mode 100755 From fc415c08f851294281663acd91cdb5537a6b77e8 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 13:53:02 -0700 Subject: [PATCH 02/25] sketch out constructor and _to_string method --- ccx_keys/locator.py | 76 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 75 insertions(+), 1 deletion(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 641843c..5039554 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -1,6 +1,80 @@ # -*- coding: utf-8 -*- +import re + +from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseLocator class CCXLocator(CourseLocator): - pass + """Concrete implementation of an Opaque Key for CCX courses""" + + CANONICAL_NAMESPACE = 'ccx-v1' + KEY_FIELDS = ('org', 'course', 'run', 'branch', 'version_guid', 'ccx') + __slots__ = KEY_FIELDS + CHECKED_INIT = False + CCX_PREFIX = 'ccx' + + # pep8 and pylint don't agree on the indentation in this block; let's make + # pep8 happy and ignore pylint as that's easier to do. + # pylint: disable=bad-continuation + URL_RE_SOURCE = r""" + ((?P{ALLOWED_ID_CHARS}+)\+(?P{ALLOWED_ID_CHARS}+)(\+(?P{ALLOWED_ID_CHARS}+))?{SEP})?? + ({BRANCH_PREFIX}@(?P{ALLOWED_ID_CHARS}+){SEP})? + ({VERSION_PREFIX}@(?P[A-F0-9]+){SEP})? + ({CCX_PREFIX}@(?P\d+){SEP}) + ({BLOCK_TYPE_PREFIX}@(?P{ALLOWED_ID_CHARS}+){SEP})? + ({BLOCK_PREFIX}@(?P{ALLOWED_ID_CHARS}+))? + """.format( + ALLOWED_ID_CHARS=CourseLocator.ALLOWED_ID_CHARS, + BRANCH_PREFIX=CourseLocator.BRANCH_PREFIX, + VERSION_PREFIX=CourseLocator.VERSION_PREFIX, + BLOCK_TYPE_PREFIX=CourseLocator.BLOCK_TYPE_PREFIX, + BLOCK_PREFIX=CourseLocator.BLOCK_PREFIX, + CCX_PREFIX=CCX_PREFIX, + SEP=r'(\+(?=.)|$)', # Separator: requires a non-trailing '+' or end of string + ) + + URL_RE = re.compile( + '^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE + ) + + def __init__( + self, + org=None, + course=None, + run=None, + ccx=None, + branch=None, + version_guid=None, + deprecated=False, + **kwargs + ): + """constructor for a ccx locator""" + # for a ccx locator we require a ccx id to be passed. + if ccx is None: + raise InvalidKeyError(self.__class__, "ccx must be set") + + super(CCXLocator, self).__init__( + org=org, + course=course, + run=run, + ccx=ccx, + branch=branch, + version_guid=version_guid, + deprecated=deprecated, + **kwargs + ) + + def _to_string(self): + """ + Return a string representing this location. + """ + parts = [] + if self.course and self.run: + parts.extend([self.org, self.course, self.run]) + if self.branch: + parts.append(u"{prefix}@{branch}".format(prefix=self.BRANCH_PREFIX, branch=self.branch)) + if self.version_guid: + parts.append(u"{prefix}@{guid}".format(prefix=self.VERSION_PREFIX, guid=self.version_guid)) + parts.append(u"{prefix}@{ccx}".format(prefix=self.CCX_PREFIX, ccx=self.ccx)) + return u"+".join(parts) From f782eb489014d82162a1894c0d3017cdcad63c03 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 13:53:21 -0700 Subject: [PATCH 03/25] the key will have a 'ccx' property --- ccx_keys/key.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ccx_keys/key.py b/ccx_keys/key.py index 268f65b..d25ede4 100644 --- a/ccx_keys/key.py +++ b/ccx_keys/key.py @@ -1,6 +1,11 @@ # -*- coding: utf-8 -*- +from abc import abstractproperty from opaque_keys.edx.keys import CourseKey class CCXKey(CourseKey): - pass + + @abstractproperty + def ccx(self): + """The id of the ccx for this key""" + raise NotImplementedError() From 14b0d99e6cf0720f9be8332d8876042898af3761 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 13:53:45 -0700 Subject: [PATCH 04/25] build tests for the constructor (and for _to_string) --- ccx_keys/tests/test_ccx_keys.py | 140 +++++++++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 4 deletions(-) diff --git a/ccx_keys/tests/test_ccx_keys.py b/ccx_keys/tests/test_ccx_keys.py index 8f65577..3e299c9 100644 --- a/ccx_keys/tests/test_ccx_keys.py +++ b/ccx_keys/tests/test_ccx_keys.py @@ -1,7 +1,139 @@ -from opaque_keys.edx.tests import LocatorBaseTest +import ddt +from bson.objectid import ObjectId +from opaque_keys import InvalidKeyError +from opaque_keys.edx.tests import LocatorBaseTest, TestDeprecated +from ccx_keys.locator import CCXLocator -class TestCCXKeys(LocatorBaseTest): - def test_foo(self): - self.fail("not a real test") +@ddt.ddt +class TestCCXKeys(LocatorBaseTest, TestDeprecated): + """ + Tests of :class:`.CCXKey` and :class:`.CCXLocator` + """ + + def test_ccx_constructor_package_id(self): + """Verify a locator constructed without branch or version is correct""" + org = 'mit.eecs' + course = '6002x' + run = '2014_T2' + ccx = 1 + testurn = '{}+{}+{}+{}@{}'.format( + org, course, run, CCXLocator.CCX_PREFIX, ccx + ) + testobj = CCXLocator(org=org, course=course, run=run, ccx=ccx) + + self.check_course_locn_fields( + testobj, org=org, course=course, run=run + ) + self.assertEqual(testobj.ccx, ccx) + # Allow access to _to_string + # pylint: disable=protected-access + self.assertEqual(testobj._to_string(), testurn) + + def test_ccx_constructor_version_guid(self): + """Verify a locator constructed with only version_guid is correct""" + test_id_loc = '519665f6223ebd6980884f2b' + ccx = 1 + expected_urn = '{}@{}+{}@{}'.format( + CCXLocator.VERSION_PREFIX, test_id_loc, + CCXLocator.CCX_PREFIX, ccx + ) + testobj = CCXLocator(version_guid=test_id_loc, ccx=ccx) + + self.check_course_locn_fields( + testobj, + version_guid=ObjectId(test_id_loc), + ) + self.assertEqual(testobj.ccx, ccx) + # Allow access to _to_string + # pylint: disable=protected-access + self.assertEqual(testobj._to_string(), expected_urn) + + def test_ccx_constructor_package_id_separate_branch(self): + """Verify a locator constructed with branch is correct""" + org = 'mit.eecs' + course = '6002x' + run = '2014_T2' + test_branch = 'published' + ccx = 1 + expected_urn = '{}+{}+{}+{}@{}+{}@{}'.format( + org, course, run, + CCXLocator.BRANCH_PREFIX, test_branch, + CCXLocator.CCX_PREFIX, ccx + ) + testobj = CCXLocator( + org=org, course=course, run=run, branch=test_branch, ccx=ccx + ) + + self.check_course_locn_fields( + testobj, + org=org, + course=course, + run=run, + branch=test_branch, + ) + self.assertEqual(testobj.ccx, ccx) + # Allow access to _to_string + # pylint: disable=protected-access + self.assertEqual(testobj._to_string(), expected_urn) + + def test_ccx_constructor_package_id_branch_and_version_guid(self): + """Verify a locator constructed with branch and version is correct""" + test_id_loc = '519665f6223ebd6980884f2b' + org = 'mit.eecs' + course = '~6002x' + run = '2014_T2' + branch = 'draft-1' + ccx = 1 + expected_urn = '{}+{}+{}+{}@{}+{}@{}+{}@{}'.format( + org, course, run, + CCXLocator.BRANCH_PREFIX, branch, + CCXLocator.VERSION_PREFIX, test_id_loc, + CCXLocator.CCX_PREFIX, ccx + ) + testobj = CCXLocator( + org=org, + course=course, + run=run, + branch=branch, + version_guid=test_id_loc, + ccx=ccx + ) + + self.check_course_locn_fields( + testobj, + org=org, + course=course, + run=run, + branch=branch, + version_guid=ObjectId(test_id_loc) + ) + self.assertEqual(testobj.ccx, ccx) + # Allow access to _to_string + # pylint: disable=protected-access + self.assertEqual(testobj._to_string(), expected_urn) + + @ddt.data( + ('version_guid'), + ('org', 'course', 'run'), + ('org', 'course', 'run', 'branch'), + ('org', 'course', 'run', 'version_guid'), + ('org', 'course', 'run', 'branch', 'version_guid'), + ) + def test_missing_ccx_id(self, fields): + """Verify that otherwise valid arguments fail without ccx""" + available_fields = { + 'version_guid': '519665f6223ebd6980884f2b', + 'org': 'mit.eecs', + 'course': '6002x', + 'run': '2014_T2', + 'branch': 'draft-1', + } + use_fields = dict( + (k, v) for k, v in available_fields.items() if k in fields + ) + with self.assertRaises(InvalidKeyError) as cm: + CCXLocator(**use_fields) + + self.assertTrue(str(CCXLocator) in str(cm.exception)) From 07bcd5868c76b5586112dda83a9655a79b9c16b8 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 17:03:37 -0700 Subject: [PATCH 05/25] fix malformed entry point syntax --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 629e8e1..0c09c66 100644 --- a/setup.py +++ b/setup.py @@ -11,7 +11,7 @@ ], entry_points={ 'course_key': [ - 'ccx-v1 = ccx_keys.locator.CCXLocator', + 'ccx-v1 = ccx_keys.locator:CCXLocator', ], } From d98a16136851d508047b393b301079f49ced1af9 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 17:25:14 -0700 Subject: [PATCH 06/25] add an alternate constructor that will build a ccx locator from a course locator and a ccx id. --- ccx_keys/locator.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 5039554..26315d0 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -4,8 +4,10 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseLocator +from ccx_keys.key import CCXKey -class CCXLocator(CourseLocator): + +class CCXLocator(CourseLocator, CCXKey): """Concrete implementation of an Opaque Key for CCX courses""" CANONICAL_NAMESPACE = 'ccx-v1' @@ -65,6 +67,20 @@ def __init__( **kwargs ) + @classmethod + def from_course_locator(cls, course_locator, ccx): + """Construct a CCXLocator given a CourseLocator and a ccx id""" + new_obj = cls( + org=course_locator.org, + course=course_locator.course, + run=course_locator.run, + branch=course_locator.branch, + version_guid=course_locator.version_guid, + deprecated=course_locator.deprecated, + ccx=ccx, + ) + return new_obj + def _to_string(self): """ Return a string representing this location. From 474011cc128ef06dcfb491885dca636799c37f8a Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 17:26:12 -0700 Subject: [PATCH 07/25] add more tests --- ccx_keys/tests/test_ccx_keys.py | 76 +++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 4 deletions(-) diff --git a/ccx_keys/tests/test_ccx_keys.py b/ccx_keys/tests/test_ccx_keys.py index 3e299c9..22b08ea 100644 --- a/ccx_keys/tests/test_ccx_keys.py +++ b/ccx_keys/tests/test_ccx_keys.py @@ -1,6 +1,8 @@ import ddt from bson.objectid import ObjectId from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.locator import CourseLocator from opaque_keys.edx.tests import LocatorBaseTest, TestDeprecated from ccx_keys.locator import CCXLocator @@ -17,7 +19,7 @@ def test_ccx_constructor_package_id(self): org = 'mit.eecs' course = '6002x' run = '2014_T2' - ccx = 1 + ccx = '1' testurn = '{}+{}+{}+{}@{}'.format( org, course, run, CCXLocator.CCX_PREFIX, ccx ) @@ -34,7 +36,7 @@ def test_ccx_constructor_package_id(self): def test_ccx_constructor_version_guid(self): """Verify a locator constructed with only version_guid is correct""" test_id_loc = '519665f6223ebd6980884f2b' - ccx = 1 + ccx = '1' expected_urn = '{}@{}+{}@{}'.format( CCXLocator.VERSION_PREFIX, test_id_loc, CCXLocator.CCX_PREFIX, ccx @@ -56,7 +58,7 @@ def test_ccx_constructor_package_id_separate_branch(self): course = '6002x' run = '2014_T2' test_branch = 'published' - ccx = 1 + ccx = '1' expected_urn = '{}+{}+{}+{}@{}+{}@{}'.format( org, course, run, CCXLocator.BRANCH_PREFIX, test_branch, @@ -85,7 +87,7 @@ def test_ccx_constructor_package_id_branch_and_version_guid(self): course = '~6002x' run = '2014_T2' branch = 'draft-1' - ccx = 1 + ccx = '1' expected_urn = '{}+{}+{}+{}@{}+{}@{}+{}@{}'.format( org, course, run, CCXLocator.BRANCH_PREFIX, branch, @@ -137,3 +139,69 @@ def test_missing_ccx_id(self, fields): CCXLocator(**use_fields) self.assertTrue(str(CCXLocator) in str(cm.exception)) + + @ddt.unpack + @ddt.data( + {'fields': ('version_guid',), + 'url_template': '{CANONICAL_NAMESPACE}:{VERSION_PREFIX}@{version_guid}+{CCX_PREFIX}@{ccx}', + }, + {'fields': ('org', 'course', 'run'), + 'url_template': '{CANONICAL_NAMESPACE}:{org}+{course}+{run}+{CCX_PREFIX}@{ccx}', + }, + {'fields': ('org', 'course', 'run', 'branch'), + 'url_template': '{CANONICAL_NAMESPACE}:{org}+{course}+{run}+{BRANCH_PREFIX}@{branch}+{CCX_PREFIX}@{ccx}', + }, + {'fields': ('org', 'course', 'run', 'version_guid'), + 'url_template': '{CANONICAL_NAMESPACE}:{org}+{course}+{run}+{VERSION_PREFIX}@{version_guid}+{CCX_PREFIX}@{ccx}', + }, + {'fields': ('org', 'course', 'run', 'branch', 'version_guid'), + 'url_template': '{CANONICAL_NAMESPACE}:{org}+{course}+{run}+{BRANCH_PREFIX}@{branch}+{VERSION_PREFIX}@{version_guid}+{CCX_PREFIX}@{ccx}',}, + ) + def test_locator_from_good_url(self, fields, url_template): + available_fields = { + 'version_guid': '519665f6223ebd6980884f2b', + 'org': 'mit.eecs', + 'course': '6002x', + 'run': '2014_T2', + 'branch': 'draft-1', + 'ccx': '1', + 'CANONICAL_NAMESPACE': CCXLocator.CANONICAL_NAMESPACE, + 'VERSION_PREFIX': CCXLocator.VERSION_PREFIX, + 'BRANCH_PREFIX': CCXLocator.BRANCH_PREFIX, + 'CCX_PREFIX': CCXLocator.CCX_PREFIX, + } + this_url = url_template.format(**available_fields) + testobj = CourseKey.from_string(this_url) + use_keys = dict( + (k, v) for k, v in available_fields.items() if k in fields + ) + + if 'version_guid' in use_keys: + use_keys['version_guid'] = ObjectId(use_keys['version_guid']) + self.check_course_locn_fields(testobj, **use_keys) + self.assertEqual(testobj.ccx, available_fields['ccx']) + + @ddt.data( + ('version_guid'), + ('org', 'course', 'run'), + ('org', 'course', 'run', 'branch'), + ('org', 'course', 'run', 'version_guid'), + ('org', 'course', 'run', 'branch', 'version_guid'), + ) + def test_from_course_locator_constructor(self, fields): + available_fields = { + 'version_guid': '519665f6223ebd6980884f2b', + 'org': 'mit.eecs', + 'course': '6002x', + 'run': '2014_T2', + 'branch': 'draft-1', + } + ccx = '1' + use_fields = dict((k, v) for k, v in available_fields.items() if k in fields) + course_id = CourseLocator(**use_fields) + testobj = CCXLocator.from_course_locator(course_id, ccx) + + if 'version_guid' in use_fields: + use_fields['version_guid'] = ObjectId(use_fields['version_guid']) + self.check_course_locn_fields(testobj, **use_fields) + self.assertEqual(testobj.ccx, ccx) From 9f87cfef9b4cff3112d182e428d961ca6484e736 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 17:26:26 -0700 Subject: [PATCH 08/25] not using sphinx, don't include it in requirements. --- test-requirements.txt | 1 - 1 file changed, 1 deletion(-) diff --git a/test-requirements.txt b/test-requirements.txt index a324989..5870ef6 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -5,6 +5,5 @@ pep8 pylint >= 1.1.0 ddt diff-cover >= 0.2.1 -sphinx -e . -r requirements.txt From cc5c2feec5fd2e6bcd6380f61b894815af2b06a0 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 17:45:17 -0700 Subject: [PATCH 09/25] adding travis configuration --- .travis.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 .travis.yml diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..bf1c76c --- /dev/null +++ b/.travis.yml @@ -0,0 +1,12 @@ +language: python +python: + - "2.7" +before_install: + - git fetch origin master:refs/remotes/origin/master +install: + - pip install -r test-requirements.txt + - pip install coveralls +script: + - scripts/coverage +after_success: + - coveralls From 00022d52027c849a7f04ff8317e00491cc88d488 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 17:47:58 -0700 Subject: [PATCH 10/25] incorporate travis badge in readme --- README.rst | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/README.rst b/README.rst index c1bbeb5..82d6691 100644 --- a/README.rst +++ b/README.rst @@ -2,14 +2,16 @@ Part of `edX code`_. .. _`edX code`: http://code.edx.org/ -ccx-keys -======== +ccx-keys |build-status| +======================= Ccx-keys provides implementations of the Opaque Key concept specific to Custom Courses for EdX. For more on opaque keys see `the opaque-keys package`_, `its documentation`_, or the `edx-platform wiki documentation`_ regarding opaque keys. +.. |build-status| image:: https://travis-ci.org/jazkarta/ccx-keys.png + :target: https://travis-ci.org/jazkarta/ccx-keys .. _`the opaque-keys package`: https://github.com/edx/opaque-keys .. _`its documentation`: http://opaque-keys.readthedocs.org/en/latest/ .. _`edx-platform wiki documentation`: https://github.com/edx/edx-platform/wiki/Opaque-Keys-(Locators) From 88398a62f4801666963e5e70d6f45c9c954e1a98 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 18:09:44 -0700 Subject: [PATCH 11/25] add coveralls integration --- README.rst | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/README.rst b/README.rst index 82d6691..3a114fc 100644 --- a/README.rst +++ b/README.rst @@ -2,8 +2,8 @@ Part of `edX code`_. .. _`edX code`: http://code.edx.org/ -ccx-keys |build-status| -======================= +ccx-keys |build-status| |coverage-status| +========================================= Ccx-keys provides implementations of the Opaque Key concept specific to Custom Courses for EdX. For more on opaque keys see `the opaque-keys package`_, @@ -12,6 +12,9 @@ opaque keys. .. |build-status| image:: https://travis-ci.org/jazkarta/ccx-keys.png :target: https://travis-ci.org/jazkarta/ccx-keys +.. |coverage-status| image:: https://coveralls.io/repos/jazkarta/ccx-keys/badge.svg + :target: https://coveralls.io/r/jazkarta/ccx-keys + .. _`the opaque-keys package`: https://github.com/edx/opaque-keys .. _`its documentation`: http://opaque-keys.readthedocs.org/en/latest/ .. _`edx-platform wiki documentation`: https://github.com/edx/edx-platform/wiki/Opaque-Keys-(Locators) From 65ac21c51e2266f4be6fa395badc75e256d0d19d Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 18:21:36 -0700 Subject: [PATCH 12/25] incorporate quality checking --- .pep8 | 2 + pylintrc | 293 ++++++++++++++++++++++++++++++++++ scripts/max_pep8_violations | 10 ++ scripts/max_pylint_violations | 10 ++ scripts/quality | 11 ++ 5 files changed, 326 insertions(+) create mode 100644 .pep8 create mode 100644 pylintrc create mode 100755 scripts/max_pep8_violations create mode 100755 scripts/max_pylint_violations create mode 100755 scripts/quality diff --git a/.pep8 b/.pep8 new file mode 100644 index 0000000..f93712a --- /dev/null +++ b/.pep8 @@ -0,0 +1,2 @@ +[pep8] +ignore=E501 diff --git a/pylintrc b/pylintrc new file mode 100644 index 0000000..bd2551d --- /dev/null +++ b/pylintrc @@ -0,0 +1,293 @@ +[MASTER] + +# Specify a configuration file. +#rcfile= + +# Python code to execute, usually for sys.path manipulation such as +# pygtk.require(). +#init-hook= + +# Profiled execution. +profile=no + +# Add files or directories to the blacklist. They should be base names, not +# paths. +ignore=CVS, migrations + +# Pickle collected data for later comparisons. +persistent=yes + +# List of plugins (as comma separated values of python modules names) to load, +# usually to register additional checkers. +load-plugins= + + +[MESSAGES CONTROL] + +# Enable the message, report, category or checker with the given id(s). You can +# either give multiple identifier separated by comma (,) or put this option +# multiple time. +#enable= + +# Disable the message, report, category or checker with the given id(s). You +# can either give multiple identifier separated by comma (,) or put this option +# multiple time (only on the command line, not in the configuration file where +# it should appear only once). +disable= +# Never going to use these +# I0011: Locally disabling W0232 +# C0301: Line too long +# W0141: Used builtin function 'map' +# W0142: Used * or ** magic +# R0921: Abstract class not referenced +# R0922: Abstract class is only referenced 1 times + I0011,C0301,W0141,W0142,R0921,R0922, + +# Django makes classes that trigger these +# W0232: Class has no __init__ method + W0232, + +# Might use these when the code is in better shape +# C0302: Too many lines in module +# R0201: Method could be a function +# R0901: Too many ancestors +# R0902: Too many instance attributes +# R0903: Too few public methods (1/2) +# R0904: Too many public methods +# R0911: Too many return statements +# R0912: Too many branches +# R0913: Too many arguments +# R0914: Too many local variables + C0302,R0201,R0901,R0902,R0903,R0904,R0911,R0912,R0913,R0914 + + +[REPORTS] + +# Set the output format. Available formats are text, parseable, colorized, msvs +# (visual studio) and html +output-format=text + +# Put messages in a separate file for each module / package specified on the +# command line instead of printing them on stdout. Reports (if any) will be +# written in a file name "pylint_global.[txt|html]". +files-output=no + +# Tells whether to display a full report or only the messages +reports=no + +# Python expression which should return a note less than 10 (10 is the highest +# note). You have access to the variables errors warning, statement which +# respectively contain the number of errors / warnings messages and the total +# number of statements analyzed. This is used by the global evaluation report +# (RP0004). +evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / statement) * 10) + +# Add a comment according to your evaluation note. This is used by the global +# evaluation report (RP0004). +comment=no + + +[TYPECHECK] + +# Tells whether missing members accessed in mixin class should be ignored. A +# mixin class is detected if its name ends with "mixin" (case insensitive). +ignore-mixin-members=yes + +# List of classes names for which member attributes should not be checked +# (useful for classes with attributes dynamically set). +ignored-classes=SQLObject + +# When zope mode is activated, add a predefined set of Zope acquired attributes +# to generated-members. +zope=no + +# List of members which are set dynamically and missed by pylint inference +# system, and so shouldn't trigger E0201 when accessed. Python regular +# expressions are accepted. +generated-members= + REQUEST, + acl_users, + aq_parent, + objects, + DoesNotExist, + can_read, + can_write, + get_url, + size, + content, + status_code, +# For factory_boy factories + create, + build, +# For xblocks + fields, +# For locations + tag, + org, + course, + category, + name, + revision, + +[BASIC] + +# Required attributes for module, separated by a comma +required-attributes= + +# List of builtins function names that should not be used, separated by a comma +bad-functions=map,filter,apply,input + +# Regular expression which should only match correct module names +module-rgx=(([a-z_][a-z0-9_]*)|([A-Z][a-zA-Z0-9]+))$ + +# Regular expression which should only match correct module level names +const-rgx=(([A-Z_][A-Z0-9_]*)|(__.*__)|log|urlpatterns)$ + +# Regular expression which should only match correct class names +class-rgx=[A-Z_][a-zA-Z0-9]+$ + +# Regular expression which should only match correct function names +function-rgx=([a-z_][a-z0-9_]{2,30}|test_[a-z0-9_]+)$ + +# Regular expression which should only match correct method names +method-rgx=([a-z_][a-z0-9_]{2,60}|setUp|set[Uu]pClass|tearDown|tear[Dd]ownClass|assert[A-Z]\w*|maxDiff|test_[a-z0-9_]+)$ + +# Regular expression which should only match correct instance attribute names +attr-rgx=[a-z_][a-z0-9_]{2,30}$ + +# Regular expression which should only match correct argument names +argument-rgx=[a-z_][a-z0-9_]{2,30}$ + +# Regular expression which should only match correct variable names +variable-rgx=[a-z_][a-z0-9_]{2,30}$ + +# Regular expression which should only match correct list comprehension / +# generator expression variable names +inlinevar-rgx=[A-Za-z_][A-Za-z0-9_]*$ + +# Good variable names which should always be accepted, separated by a comma +good-names=f,i,j,k,db,ex,Run,_,__ + +# Bad variable names which should always be refused, separated by a comma +bad-names=foo,bar,baz,toto,tutu,tata + +# Regular expression which should only match functions or classes name which do +# not require a docstring +no-docstring-rgx=__.*__|test_.*|setUp|tearDown + + +[MISCELLANEOUS] + +# List of note tags to take in consideration, separated by a comma. +notes=FIXME,XXX,TODO + + +[FORMAT] + +# Maximum number of characters on a single line. +max-line-length=120 + +# Maximum number of lines in a module +max-module-lines=1000 + +# String used as indentation unit. This is usually " " (4 spaces) or "\t" (1 +# tab). +indent-string=' ' + + +[SIMILARITIES] + +# Minimum lines number of a similarity. +min-similarity-lines=4 + +# Ignore comments when computing similarities. +ignore-comments=yes + +# Ignore docstrings when computing similarities. +ignore-docstrings=yes + + +[VARIABLES] + +# Tells whether we should check for unused import in __init__ files. +init-import=no + +# A regular expression matching the beginning of the name of dummy variables +# (i.e. not used). +dummy-variables-rgx=_|dummy|unused|.*_unused + +# List of additional names supposed to be defined in builtins. Remember that +# you should avoid to define new builtins when possible. +additional-builtins= + + +[IMPORTS] + +# Deprecated modules which should not be used, separated by a comma +deprecated-modules=regsub,string,TERMIOS,Bastion,rexec + +# Create a graph of every (i.e. internal and external) dependencies in the +# given file (report RP0402 must not be disabled) +import-graph= + +# Create a graph of external dependencies in the given file (report RP0402 must +# not be disabled) +ext-import-graph= + +# Create a graph of internal dependencies in the given file (report RP0402 must +# not be disabled) +int-import-graph= + + +[DESIGN] + +# Maximum number of arguments for function / method +max-args=5 + +# Argument names that match this expression will be ignored. Default to name +# with leading underscore +ignored-argument-names=_.* + +# Maximum number of locals for function / method body +max-locals=15 + +# Maximum number of return / yield for function / method body +max-returns=6 + +# Maximum number of branch for function / method body +max-branchs=12 + +# Maximum number of statements in function / method body +max-statements=50 + +# Maximum number of parents for a class (see R0901). +max-parents=7 + +# Maximum number of attributes for a class (see R0902). +max-attributes=7 + +# Minimum number of public methods for a class (see R0903). +min-public-methods=2 + +# Maximum number of public methods for a class (see R0904). +max-public-methods=20 + + +[CLASSES] + +# List of interface methods to ignore, separated by a comma. This is used for +# instance to not check methods defines in Zope's Interface base class. +ignore-iface-methods=isImplementedBy,deferred,extends,names,namesAndDescriptions,queryDescriptionFor,getBases,getDescriptionFor,getDoc,getName,getTaggedValue,getTaggedValueTags,isEqualOrExtendedBy,setTaggedValue,isImplementedByInstancesOf,adaptWith,is_implemented_by + +# List of method names used to declare (i.e. assign) instance attributes. +defining-attr-methods=__init__,__new__,setUp + +# List of valid names for the first argument in a class method. +valid-classmethod-first-arg=cls + + +[EXCEPTIONS] + +# Exceptions that will emit a warning when being caught. Defaults to +# "Exception" +overgeneral-exceptions=Exception diff --git a/scripts/max_pep8_violations b/scripts/max_pep8_violations new file mode 100755 index 0000000..96549fe --- /dev/null +++ b/scripts/max_pep8_violations @@ -0,0 +1,10 @@ +#!/bin/bash +pep8 opaque_keys | tee /tmp/pep8-ok.log +echo +ERR=`cat /tmp/pep8-ok.log | wc -l` +if [ $ERR -ge $1 ]; then + echo "Too many pep8 violations: $ERR (max is $1)" + exit 1 +else + echo "$ERR Total pep8 Violations (max is $1)" +fi diff --git a/scripts/max_pylint_violations b/scripts/max_pylint_violations new file mode 100755 index 0000000..c5fde69 --- /dev/null +++ b/scripts/max_pylint_violations @@ -0,0 +1,10 @@ +#!/bin/bash +pylint opaque_keys | tee /tmp/pylint-ok.log +echo +ERR=`grep -E "^[C|R|W|E]:" /tmp/pylint-ok.log | wc -l` +if [ $ERR -ge $1 ]; then + echo "Too many pylint violations: $ERR (max is $1)" + exit 1 +else + echo "$ERR Total Pylint Violations (max is $1)" +fi diff --git a/scripts/quality b/scripts/quality new file mode 100755 index 0000000..317c54a --- /dev/null +++ b/scripts/quality @@ -0,0 +1,11 @@ +# Compute quality +if [ "$1" == "html" ]; then + echo "Generating diff_quality_pep8.html" + diff-quality --violations=pep8 --html-report diff_quality_pep8.html + echo "Generating diff_quality_pylint.html" + diff-quality --violations=pylint --html-report diff_quality_pylint.html +else + diff-quality --violations=pep8 + echo + diff-quality --violations=pylint +fi From 4f3b2f6f0192b5723af118f7a841f32fa8f157d8 Mon Sep 17 00:00:00 2001 From: cewing Date: Fri, 15 May 2015 18:22:03 -0700 Subject: [PATCH 13/25] code quality --- ccx_keys/locator.py | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 26315d0..2fa84e4 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -89,8 +89,21 @@ def _to_string(self): if self.course and self.run: parts.extend([self.org, self.course, self.run]) if self.branch: - parts.append(u"{prefix}@{branch}".format(prefix=self.BRANCH_PREFIX, branch=self.branch)) + parts.append( + # pylint: disable=no-member + u"{prefix}@{branch}".format( + prefix=self.BRANCH_PREFIX, branch=self.branch + ) + ) if self.version_guid: - parts.append(u"{prefix}@{guid}".format(prefix=self.VERSION_PREFIX, guid=self.version_guid)) - parts.append(u"{prefix}@{ccx}".format(prefix=self.CCX_PREFIX, ccx=self.ccx)) + parts.append( + # pylint: disable=no-member + u"{prefix}@{guid}".format( + prefix=self.VERSION_PREFIX, guid=self.version_guid + ) + ) + parts.append( + u"{prefix}@{ccx}".format( + prefix=self.CCX_PREFIX, ccx=self.ccx) + ) return u"+".join(parts) From eaa0af9020a8cb768ff1008c78503c235e497c86 Mon Sep 17 00:00:00 2001 From: cewing Date: Mon, 18 May 2015 19:20:20 -0700 Subject: [PATCH 14/25] fix issues identified in quick code review --- ccx_keys/locator.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 2fa84e4..47ecf53 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -2,7 +2,8 @@ import re from opaque_keys import InvalidKeyError -from opaque_keys.edx.locator import CourseLocator +from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator +from opaque_keys.edx.keys import UsageKey from ccx_keys.key import CCXKey @@ -11,7 +12,7 @@ class CCXLocator(CourseLocator, CCXKey): """Concrete implementation of an Opaque Key for CCX courses""" CANONICAL_NAMESPACE = 'ccx-v1' - KEY_FIELDS = ('org', 'course', 'run', 'branch', 'version_guid', 'ccx') + KEY_FIELDS = CourseLocator.KEY_FIELDS + ('ccx', ) __slots__ = KEY_FIELDS CHECKED_INIT = False CCX_PREFIX = 'ccx' @@ -45,7 +46,6 @@ def __init__( org=None, course=None, run=None, - ccx=None, branch=None, version_guid=None, deprecated=False, @@ -53,14 +53,13 @@ def __init__( ): """constructor for a ccx locator""" # for a ccx locator we require a ccx id to be passed. - if ccx is None: + if 'ccx' not in kwargs: raise InvalidKeyError(self.__class__, "ccx must be set") super(CCXLocator, self).__init__( org=org, course=course, run=run, - ccx=ccx, branch=branch, version_guid=version_guid, deprecated=deprecated, From da385e0054d076b8c8669b1f4b604838c5e3f6f8 Mon Sep 17 00:00:00 2001 From: cewing Date: Mon, 18 May 2015 19:22:25 -0700 Subject: [PATCH 15/25] implement a block usage locator that can use CCXLocator instead of CourseLocator as the course_key value --- ccx_keys/locator.py | 79 ++++++++++++++----- ccx_keys/tests/test_ccx_keys.py | 131 +++++++++++++++++++++++++++++++- setup.py | 3 + 3 files changed, 191 insertions(+), 22 deletions(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 47ecf53..506c33d 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -84,25 +84,62 @@ def _to_string(self): """ Return a string representing this location. """ - parts = [] - if self.course and self.run: - parts.extend([self.org, self.course, self.run]) - if self.branch: - parts.append( - # pylint: disable=no-member - u"{prefix}@{branch}".format( - prefix=self.BRANCH_PREFIX, branch=self.branch - ) - ) - if self.version_guid: - parts.append( - # pylint: disable=no-member - u"{prefix}@{guid}".format( - prefix=self.VERSION_PREFIX, guid=self.version_guid - ) - ) - parts.append( - u"{prefix}@{ccx}".format( - prefix=self.CCX_PREFIX, ccx=self.ccx) + string = super(CCXLocator, self)._to_string() + # append the identifier for the ccx to the existing course string + string += u"+{prefix}@{ccx}".format( + prefix=self.CCX_PREFIX, ccx=self.ccx ) - return u"+".join(parts) + return string + + +class CCXBlockUsageLocator(BlockUsageLocator, UsageKey): + """Concrete implementation of a usage key for blocks in CCXs""" + + CANONICAL_NAMESPACE = 'ccx-block-v1' + + URL_RE = re.compile( + '^' + CCXLocator.URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE + ) + + def replace(self, **kwargs): + # BlockUsageLocator allows for the replacement of 'KEY_FIELDS' in 'self.course_key'. + # This includes the deprecated 'KEY_FIELDS' of CourseLocator `'revision'` and `'version'`. + course_key_kwargs = {} + for key in CCXLocator.KEY_FIELDS: + if key in kwargs: + course_key_kwargs[key] = kwargs.pop(key) + # XXX: do we need these protections against deprecated forms of these arguments? + if 'revision' in kwargs and 'branch' not in course_key_kwargs: + course_key_kwargs['branch'] = kwargs.pop('revision') + if 'version' in kwargs and 'version_guid' not in course_key_kwargs: + course_key_kwargs['version_guid'] = kwargs.pop('version') + if len(course_key_kwargs) > 0: + kwargs['course_key'] = self.course_key.replace(**course_key_kwargs) + + # XXX: do we need these protections against deprecated forms of these arguments? + # `'name'` and `'category'` are deprecated `KEY_FIELDS`. + # Their values are reassigned to the new keys. + if 'name' in kwargs and 'block_id' not in kwargs: + kwargs['block_id'] = kwargs.pop('name') + if 'category' in kwargs and 'block_type' not in kwargs: + kwargs['block_type'] = kwargs.pop('category') + return super(CCXBlockUsageLocator, self).replace(**kwargs) + + @classmethod + def _from_string(cls, serialized): + """ + Requests CCXLocator to deserialize its part and then adds the local + deserialization of block + """ + # Allow access to _from_string protected method + course_key = CCXLocator._from_string(serialized) # pylint: disable=protected-access + parsed_parts = cls.parse_url(serialized) + block_id = parsed_parts.get('block_id', None) + if block_id is None: + raise InvalidKeyError(cls, serialized) + return cls(course_key, parsed_parts.get('block_type'), block_id) + + @property + def ccx(self): + """Returns the ccx for this object's course_key.""" + return self.course_key.ccx diff --git a/ccx_keys/tests/test_ccx_keys.py b/ccx_keys/tests/test_ccx_keys.py index 22b08ea..d484342 100644 --- a/ccx_keys/tests/test_ccx_keys.py +++ b/ccx_keys/tests/test_ccx_keys.py @@ -1,11 +1,13 @@ import ddt from bson.objectid import ObjectId +from itertools import product from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey +from opaque_keys.edx.keys import CourseKey, UsageKey from opaque_keys.edx.locator import CourseLocator from opaque_keys.edx.tests import LocatorBaseTest, TestDeprecated from ccx_keys.locator import CCXLocator +from ccx_keys.locator import CCXBlockUsageLocator @ddt.ddt @@ -205,3 +207,130 @@ def test_from_course_locator_constructor(self, fields): use_fields['version_guid'] = ObjectId(use_fields['version_guid']) self.check_course_locn_fields(testobj, **use_fields) self.assertEqual(testobj.ccx, ccx) + + +@ddt.ddt +class TestCCXBlockUsageLocator(LocatorBaseTest): + """ + Tests of :class:`.CCXBlockUsageLocator` + """ + @ddt.data( + # do we need or even want to support deprecated forms of urls? + "ccx-block-v1:org+course+run+ccx@1+{}@category+{}@name".format(CCXBlockUsageLocator.BLOCK_TYPE_PREFIX, CCXBlockUsageLocator.BLOCK_PREFIX), + "ccx-block-v1:org+course+run+{}@revision+ccx@1+{}@category+{}@name".format(CourseLocator.BRANCH_PREFIX, CCXBlockUsageLocator.BLOCK_TYPE_PREFIX, CCXBlockUsageLocator.BLOCK_PREFIX), + "i4x://org/course/category/name@revision", + # now try the extended char sets - we expect that "%" should be OK in deprecated-style ids, + # but should not be valid in new-style ids + "ccx-block-v1:org.dept.sub-prof+course.num.section-4+run.hour.min-99+ccx@1+{}@category+{}@name:12.33-44".format(CCXBlockUsageLocator.BLOCK_TYPE_PREFIX, CCXBlockUsageLocator.BLOCK_PREFIX), + "i4x://org.dept%sub-prof/course.num%section-4/category/name:12%33-44", + ) + def test_string_roundtrip(self, url): + actual = unicode(UsageKey.from_string(url)) + self.assertEquals( + url, + actual + ) + + @ddt.data( + "ccx-block-v1:org+course+run+ccx@1+{}@category".format(CCXBlockUsageLocator.BLOCK_TYPE_PREFIX), + "ccx-block-v1:org+course+run+{}@revision+ccx@1+{}@category".format(CourseLocator.BRANCH_PREFIX, CCXBlockUsageLocator.BLOCK_TYPE_PREFIX), + ) + def test_missing_block_id(self, url): + with self.assertRaises(InvalidKeyError): + UsageKey.from_string(url) + + @ddt.data( + ((), { + 'org': 'org', + 'course': 'course', + 'run': 'run', + 'ccx': '1', + 'category': 'category', + 'name': 'name', + }, 'org', 'course', 'run', '1', 'category', 'name', None), + ((), { + 'org': 'org', + 'course': 'course', + 'run': 'run', + 'ccx': '1', + 'category': 'category', + 'name': 'name:more_name', + }, 'org', 'course', 'run', '1', 'category', 'name:more_name', None), + ([], {}, 'org', 'course', 'run', '1', 'category', 'name', None), + ) + @ddt.unpack + def test_valid_locations(self, args, kwargs, org, course, run, ccx, category, name, revision): # pylint: disable=unused-argument + course_key = CCXLocator(org=org, course=course, run=run, branch=revision, ccx=ccx) + locator = CCXBlockUsageLocator(course_key, block_type=category, block_id=name, ) + self.assertEquals(org, locator.org) + self.assertEquals(course, locator.course) + self.assertEquals(run, locator.run) + self.assertEquals(ccx, locator.ccx) + self.assertEquals(category, locator.block_type) + self.assertEquals(name, locator.block_id) + self.assertEquals(revision, locator.branch) + + @ddt.data( + (("foo",), {}), + (["foo", "bar"], {}), + (["foo", "bar", "baz", "blat/blat", "foo"], {}), + (["foo", "bar", "baz", "blat", "foo/bar"], {}), + (["foo", "bar", "baz", "blat:blat", "foo:bar"], {}), # ':' ok in name, not in category + (('org', 'course', 'run', 'category', 'name with spaces', 'revision'), {}), + (('org', 'course', 'run', 'category', 'name/with/slashes', 'revision'), {}), + (('org', 'course', 'run', 'category', 'name', u'\xae'), {}), + (('org', 'course', 'run', 'category', u'\xae', 'revision'), {}), + ((), { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name@more_name', + 'org': 'org' + }), + ((), { + 'tag': 'tag', + 'course': 'course', + 'category': 'category', + 'name': 'name ', # extra space + 'org': 'org' + }), + ) + @ddt.unpack + def test_invalid_locations(self, *args, **kwargs): + with self.assertRaises(TypeError): + CCXBlockUsageLocator(*args, **kwargs) + + + + @ddt.data( + ('course', 'newvalue'), + ('org', 'newvalue'), + ('run', 'newvalue'), + ('branch', 'newvalue'), + ('version_guid', ObjectId('519665f6223ebd6980884f2b')), + ('block_id', 'newvalue'), + ('block_type', 'newvalue'), + ('ccx', '2'), + ) + @ddt.unpack + def test_replacement(self, key, newvalue): + course_key = CCXLocator('org', 'course', 'run', 'rev', ccx='1', deprecated=False) + kwargs = {key: newvalue} + self.assertEquals( + getattr(CCXBlockUsageLocator(course_key, 'c', 'n', deprecated=False).replace(**kwargs), key), + newvalue + ) + + with self.assertRaises(InvalidKeyError): + CCXBlockUsageLocator(course_key, 'c', 'n', deprecated=True).replace(block_id=u'name\xae') + + @ddt.data(*product((True, False), repeat=2)) + @ddt.unpack + def test_map_into_course_location(self, deprecated_source, deprecated_dest): + original_course = CCXLocator('org', 'course', 'run', ccx='1', deprecated=deprecated_source) + new_course = CCXLocator('edX', 'toy', '2012_Fall', ccx='1', deprecated=deprecated_dest) + loc = CCXBlockUsageLocator(original_course, 'cat', 'name:more_name', deprecated=deprecated_source) + expected = CCXBlockUsageLocator(new_course, 'cat', 'name:more_name', deprecated=deprecated_dest) + actual = loc.map_into_course(new_course) + + self.assertEquals(expected, actual) diff --git a/setup.py b/setup.py index 0c09c66..cbf7ef7 100644 --- a/setup.py +++ b/setup.py @@ -13,6 +13,9 @@ 'course_key': [ 'ccx-v1 = ccx_keys.locator:CCXLocator', ], + 'usage_key': [ + 'ccx-block-v1 = ccx_keys.locator:CCXBlockUsageLocator', + ] } ) From d1c6b2e71315b6c6a45be89912e895546d422765 Mon Sep 17 00:00:00 2001 From: cewing Date: Wed, 20 May 2015 19:30:29 -0700 Subject: [PATCH 16/25] prevent deprecated keys from interoperating with CCX locators of any kind --- ccx_keys/locator.py | 33 ++++++++++++++--------- ccx_keys/tests/test_ccx_keys.py | 46 ++++++++++++++++++++++++++++----- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 506c33d..6fca78a 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -56,6 +56,9 @@ def __init__( if 'ccx' not in kwargs: raise InvalidKeyError(self.__class__, "ccx must be set") + if deprecated: + raise InvalidKeyError(self.__class__, "cannot be deprecated") + super(CCXLocator, self).__init__( org=org, course=course, @@ -91,6 +94,15 @@ def _to_string(self): ) return string + def _to_deprecated_string(self): + """ CCXLocators are never deprecated. """ + raise NotImplementedError + + @classmethod + def _from_deprecated_string(cls, serialized): + """ CCXLocators are never deprecated. """ + raise NotImplementedError + class CCXBlockUsageLocator(BlockUsageLocator, UsageKey): """Concrete implementation of a usage key for blocks in CCXs""" @@ -108,21 +120,9 @@ def replace(self, **kwargs): for key in CCXLocator.KEY_FIELDS: if key in kwargs: course_key_kwargs[key] = kwargs.pop(key) - # XXX: do we need these protections against deprecated forms of these arguments? - if 'revision' in kwargs and 'branch' not in course_key_kwargs: - course_key_kwargs['branch'] = kwargs.pop('revision') - if 'version' in kwargs and 'version_guid' not in course_key_kwargs: - course_key_kwargs['version_guid'] = kwargs.pop('version') if len(course_key_kwargs) > 0: kwargs['course_key'] = self.course_key.replace(**course_key_kwargs) - # XXX: do we need these protections against deprecated forms of these arguments? - # `'name'` and `'category'` are deprecated `KEY_FIELDS`. - # Their values are reassigned to the new keys. - if 'name' in kwargs and 'block_id' not in kwargs: - kwargs['block_id'] = kwargs.pop('name') - if 'category' in kwargs and 'block_type' not in kwargs: - kwargs['block_type'] = kwargs.pop('category') return super(CCXBlockUsageLocator, self).replace(**kwargs) @classmethod @@ -143,3 +143,12 @@ def _from_string(cls, serialized): def ccx(self): """Returns the ccx for this object's course_key.""" return self.course_key.ccx + + def _to_deprecated_string(self): + """ CCXBlockUsageLocators are never deprecated. """ + raise NotImplementedError + + @classmethod + def _from_deprecated_string(cls, serialized): + """ CCXBlockUsageLocators are never deprecated.""" + raise NotImplementedError diff --git a/ccx_keys/tests/test_ccx_keys.py b/ccx_keys/tests/test_ccx_keys.py index d484342..53c69a8 100644 --- a/ccx_keys/tests/test_ccx_keys.py +++ b/ccx_keys/tests/test_ccx_keys.py @@ -208,6 +208,28 @@ def test_from_course_locator_constructor(self, fields): self.check_course_locn_fields(testobj, **use_fields) self.assertEqual(testobj.ccx, ccx) + def test_ccx_constructor_deprecated(self): + """Verify that a CCXLocator cannot be built with deprecated=True""" + with self.assertRaises(InvalidKeyError): + CCXLocator(org='org', course='course', run='run', ccx='1', deprecated=True) + + def test_ccx_from_deprecated_course_locator(self): + """Verify that a CCXLocator cannot be built from a deprecated course id""" + course_id = CourseLocator(org='org', course='course', run='run', deprecated=True) + with self.assertRaises(InvalidKeyError): + CCXLocator.from_course_locator(course_id, '1') + + def test_ccx_from_deprecated_string(self): + """Verify that _from_deprecated_string raises NotImplemented""" + with self.assertRaises(NotImplementedError): + CCXLocator._from_deprecated_string('org/course/run/1') # pylint: disable=protected-access + + def test_ccx_to_deprecated_string(self): + """Verify that _from_deprecated_string raises NotImplemented""" + testme = CCXLocator(org='org', course='course', run='run', ccx='1') + with self.assertRaises(NotImplementedError): + testme._to_deprecated_string() # pylint: disable=protected-access + @ddt.ddt class TestCCXBlockUsageLocator(LocatorBaseTest): @@ -324,13 +346,23 @@ def test_replacement(self, key, newvalue): with self.assertRaises(InvalidKeyError): CCXBlockUsageLocator(course_key, 'c', 'n', deprecated=True).replace(block_id=u'name\xae') - @ddt.data(*product((True, False), repeat=2)) - @ddt.unpack - def test_map_into_course_location(self, deprecated_source, deprecated_dest): - original_course = CCXLocator('org', 'course', 'run', ccx='1', deprecated=deprecated_source) - new_course = CCXLocator('edX', 'toy', '2012_Fall', ccx='1', deprecated=deprecated_dest) - loc = CCXBlockUsageLocator(original_course, 'cat', 'name:more_name', deprecated=deprecated_source) - expected = CCXBlockUsageLocator(new_course, 'cat', 'name:more_name', deprecated=deprecated_dest) + def test_map_into_course_location(self): + original_course = CCXLocator('org', 'course', 'run', ccx='1') + new_course = CCXLocator('edX', 'toy', '2012_Fall', ccx='1') + loc = CCXBlockUsageLocator(original_course, 'cat', 'name:more_name') + expected = CCXBlockUsageLocator(new_course, 'cat', 'name:more_name') actual = loc.map_into_course(new_course) self.assertEquals(expected, actual) + + def test_ccx_from_deprecated_string(self): + """Verify that _from_deprecated_string raises NotImplemented""" + with self.assertRaises(NotImplementedError): + CCXBlockUsageLocator._from_deprecated_string('org/course/run/1') # pylint: disable=protected-access + + def test_ccx_to_deprecated_string(self): + """Verify that _from_deprecated_string raises NotImplemented""" + ccx_id = CCXLocator(org='org', course='course', run='run', ccx='1') + loc = CCXBlockUsageLocator(ccx_id, 'cat', 'name') + with self.assertRaises(NotImplementedError): + loc._to_deprecated_string() # pylint: disable=protected-access From f5b3c1d985ea0e0b06d36c3fc4e19c5bfc0ed8ca Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 01:12:32 -0700 Subject: [PATCH 17/25] fix initializer as recommended --- ccx_keys/locator.py | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 6fca78a..b15e333 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -41,33 +41,16 @@ class CCXLocator(CourseLocator, CCXKey): '^' + URL_RE_SOURCE + '$', re.IGNORECASE | re.VERBOSE | re.UNICODE ) - def __init__( - self, - org=None, - course=None, - run=None, - branch=None, - version_guid=None, - deprecated=False, - **kwargs - ): + def __init__(self, **kwargs): """constructor for a ccx locator""" # for a ccx locator we require a ccx id to be passed. if 'ccx' not in kwargs: raise InvalidKeyError(self.__class__, "ccx must be set") - if deprecated: + if kwargs.get('deprecated', False): raise InvalidKeyError(self.__class__, "cannot be deprecated") - super(CCXLocator, self).__init__( - org=org, - course=course, - run=run, - branch=branch, - version_guid=version_guid, - deprecated=deprecated, - **kwargs - ) + super(CCXLocator, self).__init__(**kwargs) @classmethod def from_course_locator(cls, course_locator, ccx): From fc0c5ea6940fa8ac127fe02f6856835a21411d85 Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 01:13:17 -0700 Subject: [PATCH 18/25] add convenience method that returns a course locator equivalent to this CCX key without the ccx part --- ccx_keys/locator.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index b15e333..37c6450 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -66,6 +66,15 @@ def from_course_locator(cls, course_locator, ccx): ) return new_obj + def to_course_locator(self): + return CCXLocator( + org=self.org, + course=self.course, + run=self.run, + branch=self.branch, + version_guid=self.version_guid + ) + def _to_string(self): """ Return a string representing this location. From 582735ce08322ce8ca3c0e659b9197cb7f0ab2c5 Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 01:17:16 -0700 Subject: [PATCH 19/25] fix tests now that we only accept kwargs --- ccx_keys/tests/test_ccx_keys.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ccx_keys/tests/test_ccx_keys.py b/ccx_keys/tests/test_ccx_keys.py index 53c69a8..32f5bf9 100644 --- a/ccx_keys/tests/test_ccx_keys.py +++ b/ccx_keys/tests/test_ccx_keys.py @@ -336,7 +336,7 @@ def test_invalid_locations(self, *args, **kwargs): ) @ddt.unpack def test_replacement(self, key, newvalue): - course_key = CCXLocator('org', 'course', 'run', 'rev', ccx='1', deprecated=False) + course_key = CCXLocator(org='org', course='course', run='run', branch='rev', ccx='1', deprecated=False) kwargs = {key: newvalue} self.assertEquals( getattr(CCXBlockUsageLocator(course_key, 'c', 'n', deprecated=False).replace(**kwargs), key), @@ -347,8 +347,8 @@ def test_replacement(self, key, newvalue): CCXBlockUsageLocator(course_key, 'c', 'n', deprecated=True).replace(block_id=u'name\xae') def test_map_into_course_location(self): - original_course = CCXLocator('org', 'course', 'run', ccx='1') - new_course = CCXLocator('edX', 'toy', '2012_Fall', ccx='1') + original_course = CCXLocator(org='org', course='course', run='run', ccx='1') + new_course = CCXLocator(org='edX', course='toy', run='2012_Fall', ccx='1') loc = CCXBlockUsageLocator(original_course, 'cat', 'name:more_name') expected = CCXBlockUsageLocator(new_course, 'cat', 'name:more_name') actual = loc.map_into_course(new_course) From 5c7355694c7e41d93373e03631f80e981f1bfa74 Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 01:23:08 -0700 Subject: [PATCH 20/25] fix error in method return value --- ccx_keys/locator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 37c6450..bf2cf7a 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -67,7 +67,7 @@ def from_course_locator(cls, course_locator, ccx): return new_obj def to_course_locator(self): - return CCXLocator( + return CourseLocator( org=self.org, course=self.course, run=self.run, From 827f29394140417f641813956b0873044705e086 Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 12:16:25 -0700 Subject: [PATCH 21/25] implement the methods responsible for constructing asset and usage keys from this ccx key. Ensures we have control over how they are created --- ccx_keys/locator.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index bf2cf7a..21c9c7e 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -2,7 +2,11 @@ import re from opaque_keys import InvalidKeyError -from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator +from opaque_keys.edx.locator import ( + AssetLocator, + BlockUsageLocator, + CourseLocator, +) from opaque_keys.edx.keys import UsageKey from ccx_keys.key import CCXKey @@ -95,6 +99,22 @@ def _from_deprecated_string(cls, serialized): """ CCXLocators are never deprecated. """ raise NotImplementedError + def make_usage_key(self, block_type, block_id): + return CCXBlockUsageLocator( + course_key=self, + block_type=block_type, + block_id=block_id, + deprecated=self.deprecated, + ) + + def make_asset_key(self, asset_type, path): + return AssetLocator( + self.to_course_locator, + asset_type, + path, + deprecated=self.deprecated + ) + class CCXBlockUsageLocator(BlockUsageLocator, UsageKey): """Concrete implementation of a usage key for blocks in CCXs""" From fe661bd77ab37fdcca03c9839032fadf2f742fca Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 12:20:27 -0700 Subject: [PATCH 22/25] of course, ccx keys do not have the deprecated attribute, maybe they should --- ccx_keys/locator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 21c9c7e..7a18fd9 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -104,7 +104,7 @@ def make_usage_key(self, block_type, block_id): course_key=self, block_type=block_type, block_id=block_id, - deprecated=self.deprecated, + deprecated=False ) def make_asset_key(self, asset_type, path): @@ -112,7 +112,7 @@ def make_asset_key(self, asset_type, path): self.to_course_locator, asset_type, path, - deprecated=self.deprecated + deprecated=False ) From 633b28f1ff6394aab03e6916da6152bcd2fc11ed Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 12:21:26 -0700 Subject: [PATCH 23/25] ccx locators are *never* deprecated --- ccx_keys/locator.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 7a18fd9..b631fe9 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -20,6 +20,7 @@ class CCXLocator(CourseLocator, CCXKey): __slots__ = KEY_FIELDS CHECKED_INIT = False CCX_PREFIX = 'ccx' + deprecated = False # pep8 and pylint don't agree on the indentation in this block; let's make # pep8 happy and ignore pylint as that's easier to do. From 81d071ae40b2473cd6ef5ca98fc30915ea75bd1f Mon Sep 17 00:00:00 2001 From: cewing Date: Sat, 30 May 2015 12:26:11 -0700 Subject: [PATCH 24/25] call the method Not much fun doing all this way out in the open like this. A bit embarassing --- ccx_keys/locator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index b631fe9..183c37c 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -110,7 +110,7 @@ def make_usage_key(self, block_type, block_id): def make_asset_key(self, asset_type, path): return AssetLocator( - self.to_course_locator, + self.to_course_locator(), asset_type, path, deprecated=False From e6b03704b1bb97c1d2f31301ecb4e3a687c536ea Mon Sep 17 00:00:00 2001 From: cewing Date: Mon, 1 Jun 2015 00:50:28 -0700 Subject: [PATCH 25/25] add a method to the `CCXBlockUsageLocator` class that will return a normal BlockUsageLocator. In keeping with the idea of a 'neutral' or 'course-local' version of ccx keys --- ccx_keys/locator.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ccx_keys/locator.py b/ccx_keys/locator.py index 183c37c..ad420ec 100644 --- a/ccx_keys/locator.py +++ b/ccx_keys/locator.py @@ -157,6 +157,13 @@ def ccx(self): """Returns the ccx for this object's course_key.""" return self.course_key.ccx + def to_block_locator(self): + return BlockUsageLocator( + course_key=self.course_key.to_course_locator(), + block_type=self.block_type, + block_id=self.block_id + ) + def _to_deprecated_string(self): """ CCXBlockUsageLocators are never deprecated. """ raise NotImplementedError