Review: Needs Fixing code
313 + inline_comments=Dict(key_type=TextLine()))
The key type is int. But I don't think the schema stuff can enforce this;
you'll want to validate that it's an {int: unicode} dict in the method.
343 + def getPublishedInlineComments(diff_timestamp):
I'd just call this getInlineComments now. Drafts aren't really comments until
they're saved, and API clients shouldn't have to know about draft/publish.
642 + assert diff_timestamp is not None, (
643 + 'Inline comments must be associated with a previewdiff '
644 + 'timestamp.')
This will OOPS if an API client specifies inline_comments but not
diff_timestamp. As above, you should validate and raise a 400 if the input is
invalid.
779 + if type(comments) == dict:
780 + comments = json.dumps(comments).decode('utf-8')
comments should always be a dict, and the JSON() columns should do the
json.dumps automatically AFAIK.
804 + def getDraft(self, previewdiff, person):
805 + draft_comments = []
806 + cricd = self._findDraftObject(previewdiff, person)
807 + if cricd:
808 + for lineno, comment in json.loads(cricd.comments).iteritems():
809 + draft_comments.append([lineno, None, comment, None])
810 + return draft_comments
This might as well just return dict((line, comment) for (line, comment) in
cricd.comments). The 4-tuple isn't useful for drafts, which have no timestamp
and all the same person.
808 + for lineno, comment in json.loads(cricd.comments).iteritems():
823 + comments = json.loads(cric.comments)
I'm surprised that these lines work now that the columns are defined as JSON().
I'd expect the decoding to happen automatically.
883 + def get_published(self, diff_timestamp=None):
This should be getPublished. Same with get_draft -> getDraft.
--
https://code.launchpad.net/~cprov/launchpad/cric-api-take2/+merge/200563
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~cprov/launchpad/cric-api-take2 into lp:launchpad.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
