Patchwork [5,of,7] docchecker: refactor check

login
register
mail settings
Submitter timeless@mozdev.org
Date April 4, 2016, 9:31 a.m.
Message ID <3f37daec9b438013c619.1459762291@waste.org>
Download mbox | patch
Permalink /patch/14339/
State Accepted
Headers show

Comments

timeless@mozdev.org - April 4, 2016, 9:31 a.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1456975964 0
#      Thu Mar 03 03:32:44 2016 +0000
# Node ID 3f37daec9b438013c6192e128bca472b6ec944e9
# Parent  1645eabe1a4e8a22613d49f1caf1263db5b3f7a2
docchecker: refactor check
via Mercurial-devel - April 6, 2016, 8:45 p.m.
On Mon, Apr 4, 2016 at 2:31 AM, timeless <timeless@mozdev.org> wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1456975964 0
> #      Thu Mar 03 03:32:44 2016 +0000
> # Node ID 3f37daec9b438013c6192e128bca472b6ec944e9
> # Parent  1645eabe1a4e8a22613d49f1caf1263db5b3f7a2
> docchecker: refactor check

It seems like more than a refactoring to me. If I'm reading it right,
if a line had two errors, it would print:

line with error
error 1
line with error
error 2

After this patch:

line with error
error 1
error 2

Correct? I think the new output is good, but the patch shouldn't have
been disguised as a refactoring.


> diff --git a/doc/docchecker b/doc/docchecker
> --- a/doc/docchecker
> +++ b/doc/docchecker
> @@ -10,16 +10,23 @@
>  import re
>
>  leadingline = re.compile(r'(^\s*)(\S.*)$')
> -hg_backtick = re.compile(r""":hg:`[^`]*'[^`]*`""")
> -hg_cramped = re.compile(r'\w:hg:`')
> +
> +checks = [
> +  (r""":hg:`[^`]*'[^`]*`""",
> +    """warning: please avoid nesting ' in :hg:`...`"""),
> +  (r'\w:hg:`',
> +    'warning: please have a space before :hg:'),
> +]
>
>  def check(line):
> -    if hg_backtick.search(line):
> +    messages = []
> +    for match, msg in checks:
> +        if re.search(match, line):
> +            messages.append(msg)
> +    if len(messages):

No need to wrap in len(), is there?

>          print(line)
> -        print("""warning: please avoid nesting ' in :hg:`...`""")
> -    if hg_cramped.search(line):
> -        print(line)
> -        print('warning: please have a space before :hg:')
> +        for msg in messages:
> +            print(msg)
>
>  def work(file):
>      (llead, lline) = ('', '')
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
timeless - April 6, 2016, 10:02 p.m.
Martin von Zweigbergk wrote:
> It seems like more than a refactoring to me. If I'm reading it right,
> if a line had two errors, it would print:
>
> line with error
> error 1
> line with error
> error 2
>
> After this patch:
>
> line with error
> error 1
> error 2
>
> Correct? I think the new output is good, but the patch shouldn't have
> been disguised as a refactoring.

Sorry. I meant that it refactored the reporting, not the code.

There's probably a better way to write it than what I wrote... Please
feel free to fix in flight.
via Mercurial-devel - April 6, 2016, 10:12 p.m.
On Wed, Apr 6, 2016 at 3:02 PM, timeless <timeless@gmail.com> wrote:
> Martin von Zweigbergk wrote:
>> It seems like more than a refactoring to me. If I'm reading it right,
>> if a line had two errors, it would print:
>>
>> line with error
>> error 1
>> line with error
>> error 2
>>
>> After this patch:
>>
>> line with error
>> error 1
>> error 2
>>
>> Correct? I think the new output is good, but the patch shouldn't have
>> been disguised as a refactoring.
>
> Sorry. I meant that it refactored the reporting, not the code.
>
> There's probably a better way to write it than what I wrote... Please
> feel free to fix in flight.

I'll change it to "docchecker: report context line at most once".
timeless - April 6, 2016, 10:26 p.m.
sounds good, thanks

On Wed, Apr 6, 2016 at 6:12 PM, Martin von Zweigbergk
<martinvonz@google.com> wrote:
> On Wed, Apr 6, 2016 at 3:02 PM, timeless <timeless@gmail.com> wrote:
>> Martin von Zweigbergk wrote:
>>> It seems like more than a refactoring to me. If I'm reading it right,
>>> if a line had two errors, it would print:
>>>
>>> line with error
>>> error 1
>>> line with error
>>> error 2
>>>
>>> After this patch:
>>>
>>> line with error
>>> error 1
>>> error 2
>>>
>>> Correct? I think the new output is good, but the patch shouldn't have
>>> been disguised as a refactoring.
>>
>> Sorry. I meant that it refactored the reporting, not the code.
>>
>> There's probably a better way to write it than what I wrote... Please
>> feel free to fix in flight.
>
> I'll change it to "docchecker: report context line at most once".

Patch

diff --git a/doc/docchecker b/doc/docchecker
--- a/doc/docchecker
+++ b/doc/docchecker
@@ -10,16 +10,23 @@ 
 import re
 
 leadingline = re.compile(r'(^\s*)(\S.*)$')
-hg_backtick = re.compile(r""":hg:`[^`]*'[^`]*`""")
-hg_cramped = re.compile(r'\w:hg:`')
+
+checks = [
+  (r""":hg:`[^`]*'[^`]*`""",
+    """warning: please avoid nesting ' in :hg:`...`"""),
+  (r'\w:hg:`',
+    'warning: please have a space before :hg:'),
+]
 
 def check(line):
-    if hg_backtick.search(line):
+    messages = []
+    for match, msg in checks:
+        if re.search(match, line):
+            messages.append(msg)
+    if len(messages):
         print(line)
-        print("""warning: please avoid nesting ' in :hg:`...`""")
-    if hg_cramped.search(line):
-        print(line)
-        print('warning: please have a space before :hg:')
+        for msg in messages:
+            print(msg)
 
 def work(file):
     (llead, lline) = ('', '')