RFC: Tweaking source so it passes PyDev's static checks
Matt Mackall
mpm at selenic.com
Sun Jul 13 11:22:29 CDT 2008
On Sun, 2008-07-13 at 09:51 +0200, Peter Arrenbrecht wrote:
> 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])
That method probably ought to die. Today we can instead do:
for f in ctx:
print len(ctx[f].data())
> def __contains__(self, key):
> - return self._dirstate[f] not in "?r"
> + return self._dirstate[key] not in "?r"
Oops.
> * 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:
Bleah.
> - op1, op2 = repo.dirstate.parents()
> + op1, _op2 = repo.dirstate.parents()
I'm moving things towards not using dirstate directly. This sort of
thing might become op1 = repo['.'].node(). Though at the same time,
we're also moving away from using nodes directly to just using contexts.
> 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)
Ok.
> c) Drop unused exception variables:
>
> - except IOError, err:
> + except IOError:
Ok.
> 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
Yuck.
--
Mathematics is the supreme nostalgia of our time.
More information about the Mercurial-devel
mailing list