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