launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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.