[STUD-406] Fix/cdodge/unescape quote strings #387
Conversation
…function mapping table when the value to xml serialize is of basestring type. Also a couple of drive-by pep8 fixes.
|
@cpennington @cahrens can you review. Cale, I know we had discussed cleaning up more of the unnecessarily nested code in the XML exporting in xml_module.py, but all I have time to get done today is the immediate tactical fix and I'd like this to be in the next release candidate. |
| if not isinstance(value, basestring): | ||
| raise Exception('Value {0} is not of type basestring!'.format(value)) | ||
|
|
||
| return value |
There was a problem hiding this comment.
I think I'd rather have this logic just live in serialize_field, rather than having a lambda containing an if clause in the AttrMap
There was a problem hiding this comment.
Maybe @cahrens can comment on this but I wasn't sure if we want this behavior globally or just on the 'export' feature. That's why I separated the helper functions.
If serialize_field is current - and remain so in the future - only called on export use cases, then I'm fine with moving the logic.
There was a problem hiding this comment.
serialize_field should really only be for serializing fields to xml, which is exactly when we want this behavior.
There was a problem hiding this comment.
Christina, can you confirm this? If so, then I can refactor this - as well
as updating the existing test cases.
On Fri, Jul 12, 2013 at 12:22 PM, Calen Pennington <notifications@github.com
wrote:
In common/lib/xmodule/xmodule/xml_module.py:
return json.dumps(value, cls=EdxJSONEncoder)+def serialize_string_literal(
value):
- """
- Assert that the value is a base string and - if it is - simply return it
- """
- if not isinstance(value, basestring):
raise Exception('Value {0} is not of type basestring!'.format(value))
- return value
serialize_field should really only be for serializing fields to xml,
which is exactly when we want this behavior.—
Reply to this email directly or view it on GitHubhttps://github.com/edx/edx-platform/pull/387/files#r5168720
.
There was a problem hiding this comment.
Yes, I agree this should be within serialize_field. It will also make the unit tests represent how we actually serialize strings.
|
@chrisndodge I agree about putting the String-specific serialization within the serialize_field method. |
…y return that, otherwise do a json.dumps
|
@cpennington @cahrens ok refactored into that single serialize_field() function |
|
👍 (once tests pass). BTW, I did not check out the branch and run it. |
[STUD-406] Fix/cdodge/unescape quote strings
…-followed-posts Fixed cohort visibility label when viewing "Posts I'm following"
Merge pull request openedx#376 from kawaguchi-ks/develop/cypress/fix-trans-309
* stv/test/tabs: Remove check for course tab uniqueness
update email tmpl
…-patch-django-mysql FAL-2248: Monkey-patch django db introspection to avoid performance issues
…-patch-django-mysql FAL-2248: Monkey-patch django db introspection to avoid performance issues (cherry picked from commit 213b1dc)
No description provided.