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