Patchwork [3,of,3] RFC check-code: try to detect @command/def errors

login
register
mail settings
Submitter timeless@mozdev.org
Date Nov. 25, 2015, 6:31 a.m.
Message ID <c572fb376061421d6fec.1448433100@waste.org>
Download mbox | patch
Permalink /patch/11650/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

timeless@mozdev.org - Nov. 25, 2015, 6:31 a.m.
# HG changeset patch
# User timeless <timeless@mozdev.org>
# Date 1448432398 0
#      Wed Nov 25 06:19:58 2015 +0000
# Node ID c572fb376061421d6fec149a5876b155607d04ad
# Parent  a6abc5a4a6f757cca55f6db68520f934e0fa4057
RFC check-code: try to detect @command/def errors

This would have caught the bugs that were fixed in:
3836a70f1fa5
0fb08fcd5af8

It's admittedly a bit heavy-handed, but...
I'd also like to reduce the number of whitelisted warnings in
the .t -- just as I did by offering a patch for gpg
Yuya Nishihara - Nov. 28, 2015, 2:23 p.m.
On Wed, 25 Nov 2015 00:31:40 -0600, timeless wrote:
> # HG changeset patch
> # User timeless <timeless@mozdev.org>
> # Date 1448432398 0
> #      Wed Nov 25 06:19:58 2015 +0000
> # Node ID c572fb376061421d6fec149a5876b155607d04ad
> # Parent  a6abc5a4a6f757cca55f6db68520f934e0fa4057
> RFC check-code: try to detect @command/def errors

Good idea.

> This would have caught the bugs that were fixed in:
> 3836a70f1fa5
> 0fb08fcd5af8

It seems these hashes are wrong.

> It's admittedly a bit heavy-handed, but...
> I'd also like to reduce the number of whitelisted warnings in
> the .t -- just as I did by offering a patch for gpg
> 
> diff --git a/contrib/check-code.py b/contrib/check-code.py
> --- a/contrib/check-code.py
> +++ b/contrib/check-code.py
> @@ -84,6 +84,40 @@
>      t = re.sub(r"\S", "x", m.group(2))
>      return m.group(1) + t
>  
> +repcommandstr = '^@command\\(.([^,)]+).[,)]|^def ([^(]*)\('
> +repcommandre = re.compile(repcommandstr)
> +cmdbitsre = re.compile(r'\^?([^|]+)')
> +
> +def repcommandchecker(l, state):
> +    if not 'cmd' in state:
> +        state['commands'] = {}
> +        state['defs'] = {}
> +        state['cmd'] = None
> +    match = repcommandre.search(l)
> +    cmdname, defname = match.group(1), match.group(2)
> +    result = []
> +    if cmdname:
> +        state['cmd'] = cmdname
> +        cmdname = cmdbitsre.search(cmdname).group(1)
> +        if cmdname in state['commands']:
> +            result.append("duplicate @command: %s" % cmdname)
> +        state['commands'][cmdname] = True
> +    elif defname:
> +        if defname in state['defs']:
> +            result.append("duplicate def: %s" % defname)
> +        cmd = state['cmd']
> +        if cmd:
> +            cmd_ = re.sub('[^a-z]', '', cmd)
> +            defname_ = re.sub('[^a-z]', '', defname)
> +            if not (re.search('%s(?:|_|cmd)' % cmd_, defname_) or
> +                    re.search(defname_, cmd_)):
> +                result.append("@command %s does not match def %s" %
> +                    (cmd, defname))
> +        state['cmd'] = ''
> +        state['defs'][defname] = True
> +    if len(result) == 0:
> +        return None
> +    return result

[...]

> +                p, msg, ignore, callback = pat
> +            elif len(pat) == 3:
>                  p, msg, ignore = pat
> +                callback = None
>              else:
>                  p, msg = pat
>                  ignore = None
> +                callback = None
>              if i >= nerrs:
>                  msg = "warning: " + msg
>  
>              pos = 0
>              n = 0
> +            state = {}
>              for m in p.finditer(post):
>                  if prelines is None:
>                      prelines = pre.splitlines()
> @@ -523,6 +567,17 @@
>                          print "Skipping %s for %s:%s (ignore pattern)" % (
>                              name, f, n)
>                      continue
> +
> +                if callback:
> +                    result = callback(l, state)

Because this isn't a style checking, maybe we can use the AST instead of
regexp black magic. I don't think new callback and state dict would be ideal
interface for this kind of checks.

Regards,

Patch

diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -84,6 +84,40 @@ 
     t = re.sub(r"\S", "x", m.group(2))
     return m.group(1) + t
 
+repcommandstr = '^@command\\(.([^,)]+).[,)]|^def ([^(]*)\('
+repcommandre = re.compile(repcommandstr)
+cmdbitsre = re.compile(r'\^?([^|]+)')
+
+def repcommandchecker(l, state):
+    if not 'cmd' in state:
+        state['commands'] = {}
+        state['defs'] = {}
+        state['cmd'] = None
+    match = repcommandre.search(l)
+    cmdname, defname = match.group(1), match.group(2)
+    result = []
+    if cmdname:
+        state['cmd'] = cmdname
+        cmdname = cmdbitsre.search(cmdname).group(1)
+        if cmdname in state['commands']:
+            result.append("duplicate @command: %s" % cmdname)
+        state['commands'][cmdname] = True
+    elif defname:
+        if defname in state['defs']:
+            result.append("duplicate def: %s" % defname)
+        cmd = state['cmd']
+        if cmd:
+            cmd_ = re.sub('[^a-z]', '', cmd)
+            defname_ = re.sub('[^a-z]', '', defname)
+            if not (re.search('%s(?:|_|cmd)' % cmd_, defname_) or
+                    re.search(defname_, cmd_)):
+                result.append("@command %s does not match def %s" %
+                    (cmd, defname))
+        state['cmd'] = ''
+        state['defs'][defname] = True
+    if len(result) == 0:
+        return None
+    return result
 
 testpats = [
   [
@@ -299,6 +333,11 @@ 
   # warnings
   [
     (r'(^| )pp +xxxxqq[ \n][^\n]', "add two newlines after '.. note::'"),
+    (repcommandstr,
+     "@command function should be unique and match function definition",
+     None,
+     repcommandchecker
+    ),
   ]
 ]
 
@@ -494,16 +533,21 @@ 
         prelines = None
         errors = []
         for i, pat in enumerate(pats):
-            if len(pat) == 3:
+            if len(pat) == 4:
+                p, msg, ignore, callback = pat
+            elif len(pat) == 3:
                 p, msg, ignore = pat
+                callback = None
             else:
                 p, msg = pat
                 ignore = None
+                callback = None
             if i >= nerrs:
                 msg = "warning: " + msg
 
             pos = 0
             n = 0
+            state = {}
             for m in p.finditer(post):
                 if prelines is None:
                     prelines = pre.splitlines()
@@ -523,6 +567,17 @@ 
                         print "Skipping %s for %s:%s (ignore pattern)" % (
                             name, f, n)
                     continue
+
+                if callback:
+                    result = callback(l, state)
+                    if not result:
+                        if debug:
+                            print "Skipping %s for %s:%s (bad match)" % (
+                                name, f, n)
+                        continue
+                    if debug:
+                        for e in result:
+                            errors.append((f, lineno and n + 1, l, e, ""))
                 bd = ""
                 if blame:
                     bd = 'working directory'
diff --git a/tests/test-check-code-hg.t b/tests/test-check-code-hg.t
--- a/tests/test-check-code-hg.t
+++ b/tests/test-check-code-hg.t
@@ -8,8 +8,15 @@ 
 
   $ hg locate | sed 's-\\-/-g' |
   >   xargs "$check_code" $HGTEST_DEBUG --warnings --per-file=0 || false
+  hgext/graphlog.py:52:
+   > def graphlog(ui, repo, *pats, **opts):
+   warning: @command function should be unique and match function definition
+  hgext/patchbomb.py:406:
+   > def patchbomb(ui, repo, *revs, **opts):
+   warning: @command function should be unique and match function definition
   Skipping hgext/zeroconf/Zeroconf.py it has no-che?k-code (glob)
   Skipping i18n/polib.py it has no-che?k-code (glob)
   Skipping mercurial/httpclient/__init__.py it has no-che?k-code (glob)
   Skipping mercurial/httpclient/_readers.py it has no-che?k-code (glob)
   Skipping mercurial/httpclient/socketutil.py it has no-che?k-code (glob)
+  [1]