RFC: Tweaking source so it passes PyDev's static checks

Peter Arrenbrecht peter.arrenbrecht at gmail.com
Sun Jul 13 02:51:28 CDT 2008


Hi all

I've recently gone over Hg's source with PyDev[1]. It's code
inspection turned up a few useful results. So I think it would be
valuable to go over the code with such a tool from time to time. I
would do this.

However, I am also getting a ton of false positives. These could be
eliminated by tweaking the source code as shown below. I would like to
know if this would be acceptable. If so, I would also like to add them
as guidelines to the coding style[2] as a suggestion. Meaning it'd be
cool if new code already followed them. But if not, I would just fix
it up later.

pylint[3] sounds good too, but I haven't tried it yet. If we agree
that using such tools is desirable, I will give it a try.


Examples of things uncovered:

* Two references to undefined variables (as a result of refactorings I
guess - see my other, earlier post):

     def filectxs(self):
         """generate a file context for each file in this changeset's
            manifest"""
+        mf = self.manifest()
         for f in util.sort(mf):
             yield self.filectx(f, fileid=mf[f])

     def __contains__(self, key):
-        return self._dirstate[f] not in "?r"
+        return self._dirstate[key] not in "?r"

* A few unused variables pointing to unnecessary leftovers. These are
mostly minor, but it's still better to get rid of them. Here's just a
few examples:

         lstat = os.lstat
-        cmap = self._copymap
         dmap = self._map

     if stats[3]:
-        pl = repo.parents()
         repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))

 def loadall(ui):
     result = ui.configitems("extensions")
-    for i, (name, path) in enumerate(result):
+    for name, path in result:

* Quite a few unused imports, like:

-from node import nullrev, short
-import ui, hg, util, templatefilters
+from node import nullrev


Examples of tweaks to the source code:

a) Mark unused variables with a leading _, as in:

-    for st, rev, fns in changeiter:
+    for st, rev, _fns in changeiter:

-    op1, op2 = repo.dirstate.parents()
+    op1, _op2 = repo.dirstate.parents()

b) Drop unused return values, as in:

-                    fuzz = patch.patch(tmpname, ui, strip=strip, cwd=repo.root,
-                                       files=files)
+                    patch.patch(tmpname, ui, strip=strip, cwd=repo.root,
+                                files=files)

If one wants to keep the name there for documentation purposes, it
should also be prefixed by _.

c) Drop unused exception variables:

-            except IOError, err:
+            except IOError:

d) This is likely the most controversial one: Some things are
perfectly OK, but trip up PyDev. I can tell it to ignore them by
annotations. Examples:

-    import msvcrt
+    import msvcrt #@UnresolvedImport

-            return sys.getwindowsversion()[3] == 1
+            return sys.getwindowsversion()[3] == 1 #@UndefinedVariable

-            msvcrt.setmode(fd.fileno(), os.O_BINARY)
+            msvcrt.setmode(fd.fileno(), os.O_BINARY) #@UndefinedVariable


[1] http://pydev.sourceforge.net/
[2] http://www.selenic.com/mercurial/wiki/index.cgi/BasicCodingStyle
[3] http://www.logilab.org/857

-parren


More information about the Mercurial-devel mailing list