Code review comment for lp:~abentley/launchpad/restricted-diffs

Revision history for this message
Michael Nelson (michael.nelson) wrote :

Approve, just one missing blank line below, in addition to our IRC exchange.

> === modified file 'lib/lp/code/browser/diff.py'
> --- lib/lp/code/browser/diff.py 2010-02-02 17:32:23 +0000
> +++ lib/lp/code/browser/diff.py 2010-02-18 16:06:41 +0000
> @@ -10,10 +10,18 @@
>
>
> from canonical.launchpad import _
> +from canonical.launchpad.browser.librarian import FileNavigationMixin
> +from canonical.launchpad.webapp import Navigation
> +from canonical.launchpad.webapp.publisher import canonical_url
> from canonical.launchpad.webapp.tales import ObjectFormatterAPI
> +from lp.code.interfaces.diff import IPreviewDiff
> from lp.services.browser_helpers import get_plural_text
>
>
> +class PreviewDiffNavigation(Navigation, FileNavigationMixin):
> +
> + usedfor = IPreviewDiff
> +

Just need an extra line here.

> class PreviewDiffFormatterAPI(ObjectFormatterAPI):
> """Formatter for preview diffs."""
>

> === modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
> --- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-01-12 17:34:12 +0000
> +++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2010-02-18 16:06:41 +0000
> @@ -614,10 +614,25 @@
> ... return find_tag_by_id(nopriv_browser.contents, 'review-diff')
>
> The text of the review diff is in the page.
> +
> >>> print repr(extract_text(get_review_diff()))
> u'Preview Diff\nDownload diff\n1\n...Fake Diff\u1010'
>
> +There is also a link to the diff URL, which is the preview diff URL plus
> +"+files/preview.diff"
> +
> + >>> link = get_review_diff().find('a')
> + >>> print link['href']
> + http://code.launchpad.dev/~person-name.../product-name.../branch.../+merge/.../+preview-diff/+files/preview.diff

17:20 < noodles775> abentley: your branch looks great. The only question I
have is whether there's really a need to include each segment of the url in
the doctest:
http://code.launchpad.dev/~person-name.../product-name.../branch.../+merge/.../+preview-diff/+files/preview.diff
17:21 < abentley> noodles775, I would be fine with just showing "/+preview-diff/+files/preview.diff". Would you prefer that?
17:22 < noodles775> abentley: yeah (not that I care much either way, it'd just mean it was within 78chars :))

Thanks Aaron.

review: Approve (code)

« Back to merge proposal