Patch for hg-notify that adds formatting to diff lines

[Martin Geisler]

Stephen Mullins <smullins7 at gmail.com> writes:

Hi

As Greg said, then we're in a feature freeze for Mercurial 1.5. But I
still don't like to not give feedback on your patch :-)

> This is my first attempt at contributing to this project. For the
> hg-notify extension, this patch will let users add formatting to the
> diff lines of a commit. I find this useful for adding html to the
> diffs to make them easier to read in emails.

I'm the old-fashioned kind of guy who don't like HTML emails... So my
first reaction is that something like this would be best as a new
third-party extension. That would let you use whatever template language
you like instead of hard-coding things in the notify extension.

Btw, I was wondering if there shouldn't be HTML, HEAD, TITLE, BODY, ...
tags too somewhere? Perhaps you could even hook into hgweb and steal the
generated HTML from there.

> The only thing I can't figure out is how to replace the content header
> to be html instead of plain text. I thought I could get feedback on
> the initial patch and see if anyone had an idea on how to set the
> content header based on if the formatting had been added. Would
> appreciate any feedback.

Ah, so that's another problem.

Apart from the conceptual problems, I have some coding style comments
below. Feel free to disregard them if this isn't going into the core.

> diff -r 261cc6b0f15c -r aa3c3f2a2bea hgext/notify.py
> --- a/hgext/notify.py	Thu Feb 18 23:23:17 2010 -0600
> +++ b/hgext/notify.py	Fri Feb 19 22:38:08 2010 -0800
> @@ -97,6 +97,15 @@
>      'changegroup': multiple_template,
>  }
>  
> +defdiffformat = {
> +    'diff' : '<div style="color:silver">%s</span>',
> +    'difffileold' : '<div style="color:#CC3333">%s</span>',
> +    'difffilenew' : '<div style="color:#000FF">%s</span>',
> +    'difflineplus' : '<div style="background-color:#DDFFDD;">%s</span>',
> +    'difflineminus' : '<div style="background-color:#FFDDDD;">%s</span>',
> +    'diffhunk' : '<div style="color:silver">%s</span>',
> +}

There should not be a space before the colon, please see PEP 8:

  http://www.python.org/dev/peps/pep-0008/

and contrib/check-code.py

>  class notifier(object):
>      '''email notification class.'''
>  
> @@ -247,6 +256,19 @@
>                            self.subs, msgtext)
>  
>      def diff(self, ctx, ref=None):
> +        if self.ui.has_section('notifydiff'):
> +            diffline = self.ui.config('notifydiff', 'diff') or defdiffformat['diff']
> +            difffileold = self.ui.config('notifydiff', 'difffileold') or defdiffformat['difffileold']

You're introducing some long lines, please keep them below ~75
characters or so.

>          elif maxdiff > 0 and len(difflines) > maxdiff:
> -            msg = _('\ndiffs (truncated from %d to %d lines):\n\n')
> -            self.ui.write(msg % (len(difflines), maxdiff))
> +            msg = _('%sdiffs (truncated from %d to %d lines):%s%s')

All the %s will only make life harder for translators. It would be
better to do

               msg = _('diffs (truncated from %d to %d lines):')

and add the linebreaks afterwards.

> +def formatdiff(difflines, diffline, difffileold, difffilenew, difflineplus, difflineminus, diffhunk):
> +    '''given a collection of strings representing a diff, return a list of formatted strings.
> +    the formats work as follows:
> +
> +    @param diffline : lines that start with "diff"
> +    @param difffileold : lines that start with "---"
> +    @param difffilenew : lines that start with "+++"
> +    @param difflineplus : lines that start with "@@"
> +    @param difflineminus : lines that start with "-"
> +    @param diffhunk : lines that start with "+"'''

Is that doxygen syntax? We don't use that code style here.

> +
> +    formatted = []
> +    for line in difflines:
> +        if line.startswith('diff'):
> +            line = diffline % line
> +        elif line.startswith('---'):
> +            line = difffileold % line
> +        elif line.startswith('+++'):
> +            line = difffilenew % line
> +        elif line.startswith('@@'):
> +            line = diffhunk % line
> +        elif line.startswith('-'):
> +            line = difflineminus % line
> +        elif line.startswith('+'):
> +            line = difflineplus % line
> +        formatted.append(line)
> +        return formatted

That seems wrong -- you return after the first line?

-- 
Martin Geisler

Fast and powerful revision control: http://mercurial.selenic.com/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20100301/86e76998/attachment.pgp>