[PATCH v3] releasenotes: improve parsing around bullet points

Rishabh Madan rishabhmadan96 at gmail.com
Fri Jun 23 10:55:33 EDT 2017


On Fri, Jun 23, 2017 at 4:04 PM, Yuya Nishihara <yuya at tcha.org> wrote:

> On Wed, 21 Jun 2017 18:46:09 +0200, Rishabh Madan wrote:
> > # HG changeset patch
> > # User Rishabh Madan <rishabhmadan96 at gmail.com>
> > # Date 1498063380 -7200
> > #      Wed Jun 21 18:43:00 2017 +0200
> > # Node ID 12a6c92f58909ef9d696991f49ffcbf0b004913f
> > # Parent  4107eb8a5648ad31f7fb3e95bbc8999c73a94c49
> > releasenotes: improve parsing around bullet points
>
> Generally looks good to me, but I think the loop added by this patch is
> unnecessarily complicated. Can you take a look?
>
> > diff -r 4107eb8a5648 -r 12a6c92f5890 hgext/releasenotes.py
> > --- a/hgext/releasenotes.py   Tue Jun 20 23:39:59 2017 -0700
> > +++ b/hgext/releasenotes.py   Wed Jun 21 18:43:00 2017 +0200
> > @@ -185,8 +185,9 @@
> >
> >      blocks = minirst.parse(text)[0]
> >
> > -    def gatherparagraphs(offset):
> > +    def gatherparagraphsbullets(offset, title=False):
> >          paragraphs = []
> > +        bullet_points = []
>
> This function gathers either paragraphs (title=True) or bullet_points
> (title=False), so it doesn't make sense to return (paragraphs,
> bullet_points)
> pair.
>
> > +                else:
> > +                    lines = [[l[1:].strip() for l in block['lines']]]
> > +                    if i < len(blocks) - 1:
> > +                        i += 1
> > +                        block = blocks[i]
> > +                        if block['type'] == 'paragraph':
> > +                            lines.append(block['lines'])
> >
> > -                lines = [l[1:].strip() for l in block['lines']]
> > -                paragraphs.append(lines)
> > -                continue
> > +                        while block['type'] != 'bullet' and \
> > +                                i < len(blocks) - 1:
> > +                            if block['type'] == 'section':
> > +                                break
> > +                            i += 1
> > +                            block = blocks[i]
> > +                            if block['type'] == 'paragraph':
> > +                                lines.append(block['lines'])
>
> Iterating by (i + 1) index isn't common. Perhaps this can be simply written
> as a for-loop over a list slice.
>
>                     for block in blocks[i + 1:]:
>                         if block['type'] in ('bullet', 'section'):
>                             break
>                         if block['type'] == 'paragraph':
>

I'll make the suggested changes and send a new version ASAP.

>
>
>              elif block['type'] != 'paragraph':
> >                  raise error.Abort(_('unexpected block type in release
> notes: '
> >                                      '%s') % block['type'])
> > +            if title:
> > +                paragraphs.append(block['lines'])
>
> I think it's better to raise error if title=False and if a paragraph having
> no bullet found, but that can be added later.

ᐧ
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170623/96003036/attachment.html>


More information about the Mercurial-devel mailing list