Code review comment for lp:~abentley/launchpad/no-review-diff

Hi Aaron,

This branch looks good. I would just like the two docstrings changed as discussed on IRC.

merge-conditional

-Edwin

IRC Log:

[11:02 AM] <EdwinGrubbs> abentley the docstring for test_preview_diff_text_with_no_diff() says that the review_diff should be None when there is no context.preview_diff. Is that right? I don't really understand the difference between a review diff and a preview diff or whether all the objects treat them as separate.
[11:03 AM] <abentley> EdwinGrubbs: The confusion between review diffs and preview diffs was one of the reasons we got rid of review diffs. A review diff shows the changes the branch author made. A preview diff shows what would happen if you merged that branch.
[11:04 AM] <abentley> EdwinGrubbs: The review_diff field on the view class now actually refers to the preview diff, so the docstring is accurate.
[11:05 AM] <EdwinGrubbs> abentley: is that first docstring in the diff correct, or should "review_diff" be "preview_diff". In the test you are checking view.preview_diff_text.
[11:06 AM] <abentley> EdwinGrubbs: I think review_diff should be preview_diff_text.
[11:06 AM] <EdwinGrubbs> abentley: The docstring for test_preview_diff_utf8 doesn't make any sense.
[11:07 AM] <abentley> EdwinGrubbs: It means that if the bytes are utf-8-encoded, they should be decoded to unicode as utf-8.
[11:08 AM] <EdwinGrubbs> abentley: can you clarify that in the docstring?
[11:08 AM] Activating Flood Protection
[11:08 AM] Deactivating Flood Protection
[11:08 AM] <abentley> EdwinGrubbs: "A preview_diff in utf-8 should be decoded as utf-8" ?
[11:13 AM] <EdwinGrubbs> abentley: yes, but there is no reason to say utf-8 twice, can you say "decoded into a unicode object"?
[11:14 AM] <abentley> EdwinGrubbs: The reason to say it twice is because it should not be decoded using a different encoding.
[11:14 AM] <abentley> e.g. "A preview_diff in utf-8 should be decoded as utf-8, not utf-16"
[11:15 AM] <EdwinGrubbs> abentley: ok, that makes sense, since that's what you are really trying to test.
[11:18 AM] <EdwinGrubbs> abentley: in xx-branch-merge-proposals.txt, a tag with id='review-diff' is retrieved. Is that just a case where the UI hasn't yet been updated to match the name of the attribute that is now used?
[11:19 AM] <abentley> EdwinGrubbs: Yes.