-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENT-211 add course key v2 #87
base: master
Are you sure you want to change the base?
Changes from all commits
f72a1db
11498a5
b824779
1f24d58
12086b5
b1b4a1b
7a7595a
b6f4b81
39e568f
0fd82da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,13 @@ def make_asset_key(self, asset_type, path): # pragma: no cover | |
""" | ||
raise NotImplementedError() | ||
|
||
@abstractmethod | ||
def make_course_key_v2(self): # pragma: no cover | ||
""" | ||
Returns a course key v2 object without run information. | ||
""" | ||
raise NotImplementedError() | ||
|
||
|
||
class DefinitionKey(OpaqueKey): | ||
""" | ||
|
@@ -77,6 +84,37 @@ def block_type(self): # pragma: no cover | |
raise NotImplementedError() | ||
|
||
|
||
class CourseKeyV2(OpaqueKey): | ||
""" | ||
An :class:`opaque_keys.OpaqueKey` identifying a | ||
serialized Course Key object. | ||
""" | ||
KEY_TYPE = 'course_key_v2' | ||
__slots__ = () | ||
|
||
@abstractproperty | ||
def from_course_run_key(self, course_key): # pragma: no cover | ||
""" | ||
Get course key v2 from the course run key. | ||
|
||
Arguments: | ||
course_key (:class:`CourseKey`): The course identifier. | ||
|
||
""" | ||
raise NotImplementedError() | ||
|
||
@abstractproperty | ||
def make_course_run_key(self, course_run): # pragma: no cover | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? We already have existing methods of creating course run keys. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These were my suggestion. The existing keys typically are related by giving the more general key a way to create more specific keys by hydrating additional information. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, I agree that it doesn't make much sense to have both methods. We should pick one, and stick to it. |
||
""" | ||
Get course run key (course_key) from the course key v2. | ||
|
||
Arguments: | ||
course_run (str): The course run for course run identifier. | ||
|
||
""" | ||
raise NotImplementedError() | ||
|
||
|
||
class CourseObjectMixin(object): | ||
""" | ||
An abstract :class:`opaque_keys.OpaqueKey` mixin | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really necessary given that you already have
CourseKeyV2.from_course_run_key()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpennington @clintonb just to confirm, I have to remove this method only or do I need to remove the
make_course_run_key
in classCourseKeyV2
too?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zubair-arbi: You should have some a method to go from
CourseKeyV2
toCourseKey
and back again, but only one of each. I would suggest using regular methods, rather than class methods, in general. So, probablyCourseKeyV2.make_course_run_key
, andCourseKey.make_course_key_v2
.