Patchwork [v3] releasenotes: improve parsing around bullet points

login
register
mail settings
Submitter Rishabh Madan
Date June 21, 2017, 4:46 p.m.
Message ID <12a6c92f58909ef9d696.1498063569@bunty>
Download mbox | patch
Permalink /patch/21594/
State Superseded
Headers show

Comments

Rishabh Madan - June 21, 2017, 4:46 p.m.
# HG changeset patch
# User Rishabh Madan <rishabhmadan96@gmail.com>
# Date 1498063380 -7200
#      Wed Jun 21 18:43:00 2017 +0200
# Node ID 12a6c92f58909ef9d696991f49ffcbf0b004913f
# Parent  4107eb8a5648ad31f7fb3e95bbc8999c73a94c49
releasenotes: improve parsing around bullet points

Earlier, on parsing the bullet points from existing release notes the bullet
points after the first one weren't written correctly to the notes file. This
patch makes changes to parsereleasenotesfromfile() function that introduces a new
bullet_points data structure that tracks the bullets and associated subparagraph.
It also makes necessary changes to the tests related to merging of bullets.
Yuya Nishihara - June 23, 2017, 2:04 p.m.
On Wed, 21 Jun 2017 18:46:09 +0200, Rishabh Madan wrote:
> # HG changeset patch
> # User Rishabh Madan <rishabhmadan96@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':


>              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.

Patch

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 = []
 
         for i in range(offset + 1, len(blocks)):
             block = blocks[i]
@@ -198,17 +199,35 @@ 
             elif block['type'] == 'bullet':
                 if block['indent'] != 0:
                     raise error.Abort(_('indented bullet lists not supported'))
+                if title:
+                    lines = [l[1:].strip() for l in block['lines']]
+                    paragraphs.append(lines)
+                    continue
+                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'])
+                    bullet_points.append(lines)
+                    continue
             elif block['type'] != 'paragraph':
                 raise error.Abort(_('unexpected block type in release notes: '
                                     '%s') % block['type'])
+            if title:
+                paragraphs.append(block['lines'])
 
-            paragraphs.append(block['lines'])
-
-        return paragraphs
+        return paragraphs, bullet_points
 
     currentsection = None
     for i, block in enumerate(blocks):
@@ -226,16 +245,18 @@ 
                                   title)
 
             currentsection = name
-            paragraphs = gatherparagraphs(i)
-            if paragraphs:
-                notes.addnontitleditem(currentsection, paragraphs)
+            paragraphs, bullet_points = gatherparagraphsbullets(i)
+            if bullet_points:
+                for para in bullet_points:
+                    notes.addnontitleditem(currentsection, para)
 
-        elif block['underline'] == '-':  # sub-section
-            paragraphs = gatherparagraphs(i)
-
+        elif block['underline'] == '-':
             if title == BULLET_SECTION:
-                notes.addnontitleditem(currentsection, paragraphs)
+                paragraphs, bullet_points = gatherparagraphsbullets(i)
+                for para in bullet_points:
+                    notes.addnontitleditem(currentsection, para)
             else:
+                paragraphs, bullet_points = gatherparagraphsbullets(i, True)
                 notes.addtitleditem(currentsection, title, paragraphs)
         else:
             raise error.Abort(_('unsupported section type for %s') % title)
diff -r 4107eb8a5648 -r 12a6c92f5890 tests/test-releasenotes-merging.t
--- a/tests/test-releasenotes-merging.t	Tue Jun 20 23:39:59 2017 -0700
+++ b/tests/test-releasenotes-merging.t	Wed Jun 21 18:43:00 2017 +0200
@@ -34,8 +34,7 @@ 
   
   * Fix from commit message.
 
-Processing again will no-op
-TODO this is buggy
+Processing again ignores the already added bullet.
 
   $ hg releasenotes -r . $TESTTMP/single-fix-bullet
 
@@ -45,8 +44,6 @@ 
   
   * Fix from release notes.
   
-    Fix from commit message.
-  
   * Fix from commit message.
 
   $ cd ..
@@ -111,7 +108,7 @@ 
 
   $ cd ..
 
-Bullets don't merge properly
+Bullets from rev merge with those from notes file.
 
   $ hg init bullets
   $ cd bullets
@@ -157,7 +154,7 @@ 
   
   * this is fix1.
   
-    this is fix2.
+  * this is fix2.
   
   * this is fix3.
 
diff -r 4107eb8a5648 -r 12a6c92f5890 tests/test-releasenotes-parsing.t
--- a/tests/test-releasenotes-parsing.t	Tue Jun 20 23:39:59 2017 -0700
+++ b/tests/test-releasenotes-parsing.t	Wed Jun 21 18:43:00 2017 +0200
@@ -59,7 +59,9 @@ 
   section: feature
     bullet point:
       paragraph: First bullet point. It has a single line.
+    bullet point:
       paragraph: Second bullet point. It consists of multiple lines.
+    bullet point:
       paragraph: Third bullet point. It has a single line.
 
 Bullet point without newline between items
@@ -77,8 +79,11 @@ 
   section: feature
     bullet point:
       paragraph: First bullet point
+    bullet point:
       paragraph: Second bullet point And it has multiple lines
+    bullet point:
       paragraph: Third bullet point
+    bullet point:
       paragraph: Fourth bullet point
 
 Sub-section contents are read
@@ -130,10 +135,12 @@ 
   section: feature
     bullet point:
       paragraph: Feature 1
+    bullet point:
       paragraph: Feature 2
   section: fix
     bullet point:
       paragraph: Fix 1
+    bullet point:
       paragraph: Fix 2
 
 Mixed sub-sections and bullet list
@@ -166,4 +173,5 @@ 
       paragraph: Some words about the second feature. That span multiple lines.
     bullet point:
       paragraph: Bullet item 1
+    bullet point:
       paragraph: Bullet item 2