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">
<tr>
<td>
<pre style="margin: 0; padding: 0; {{precss}}">{{ raw_diff }}</pre>
</td>
</tr>
</table>
{% 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=True
#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']=''
#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):
try:
_ = 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:
context['raw_diff']=raw_diff
else:
# attach
context['raw_diff']=attachment_message
....
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
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~----------~----~----~----~------~----~------~--~---
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
