Patch for hg-notify that adds formatting to diff lines

[Stephen Mullins]

Thanks for the feedback. I always put a space after the colon but this isn't
my project so I should abide by the module's style :)

The last line in the format diff func got indented too far by accident.

I think I'm going to keep trying to get hgweb to work for my needs and then
see if I should keep this in hg-notify or in a separate extension. The nice
thing about actually figuring out the templates is that I can have a header
template to set all the tags correctly instead of having to do all that in
the hg-notify extension, which makes everything cleaner.

The more I look at this code the more familiar I'm becoming with this
project. Yall have made some powerful pieces of code (like the templater)
and I'm hoping to be able to contribute to your project. Good luck with the
release guys.

Stephen

On Mon, Mar 1, 2010 at 2:54 PM, Martin Geisler <mg at lazybytes.net> wrote:

> 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 --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20100302/1326c2c1/attachment.htm>