On 9/30/2011 11:34 AM, Stephen Gallagher wrote:
On Fri, 2011-09-30 at 11:32 -0700, Chris Clark wrote:
One option is to print the email sent from reviewboard. If you want to
print the diffs you are likely to find sizing issues with the web page
approach with the wide side-by-side diff view.

We've modified our RB server to include the diff in the email that is
sent out for review (we've not yet worked out how to NOT include the
diffs in replies/comments). That can help a little bit but the main
advantage we've found is using the browser to see the diffs and then
immediately expand code and add comments. You could do this with
face-to-face reviews if the computer is used instead of print.
Does it send the diff in the email contents or as an attachment?

It depends :-)

It is semi-configurable, i.e. there is code that has a parameter but it is hard coded at the moment.

We default to inline (this matches our old review standards that predated re) but switch to an attachment if we have no idea what the encoding of the diff is.

We need to add a size filter, e.g. we had a monster review the other day and what should have happened is the diff should have been compressed and attached.

  If the
latter, could you share your patch with the rest of the class? :)

Right now the code is in a private git repo :-( This is an excellent reminder that we should try and make this public even if no one will want all of it. Right now what we have is NOT ready for prime time

As a taster, here is the code, it is not pep8 compliant and it isn't a diff (we're on an older version), but it should allow you to make a similar (more robust!) change:

in reviewboard/templates/notifications/review_request_email.html:

{% if raw_diff %}
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<pre style="margin: 0; padding: 0; {{precss}}">{{ raw_diff }}</pre>
{% endif %}

--- this is the inline diff option

In /reviewboard/notifications/email.py:

    context['MEDIA_SERIAL'] = settings.MEDIA_SERIAL

    # should really (add to) repository and add include raw diffs option
    # for now check scm type or always do it!
    #include_raw_diff=False ## DEBUG FIXME REMOVE!
    ## NOTE some of our mailing list do not retain (diff) attachments.....
    attachment_message='Diffs attached'
    emaildiffs_inline=True ## if False make an attachment
    #context['raw_diff']='debug raw diff FIXME REMOVE'
    if include_raw_diff:
## Other option, instead of a flag "include_raw_diff" could lazy evaluate ## http://pyds.muensterland.org/ has a nice short one (only needs minor cleanup, and I already have a cleaned up copy)
        diffset = review_request.diffset_history.diffsets.latest()
raw_diff = review_request.repository.get_scmtool().get_parser('').raw_diff(diffset)
        if emaildiffs_inline and isinstance(raw_diff, str):
_ = raw_diff.decode('us-ascii') # should check repository encoding type......
            except UnicodeDecodeError:
                # non-ASCII data in diff, email is (probably) utf8 and
                # so will not display inline. Switch to an attachment
                emaildiffs_inline=False ## if False make an attachment
attachment_message='Diffs contain non-ASCII characters/bytes, unable to determine encoding. See attachment.'
        if emaildiffs_inline:
            # attach


    message = SpiffyEmailMessage(subject.strip(), text_body, html_body,
from_email, list(to_field), list(cc_field),
                                 in_reply_to, headers)

    if include_raw_diff and not emaildiffs_inline:
        message.attach('diff.diff', raw_diff, 'text/x-diff')

Want to help the Review Board project? Donate today at 
Happy user? Let us know at http://www.reviewboard.org/users/
To unsubscribe from this group, send email to 
For more options, visit this group at 

Reply via email to