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

[Peter Arrenbrecht]

Hmm. So I'll do the accepted changes. However, to do this regularly,
it would really ease my job tremendously if I could get rid of the
false positives. So, does your "Bleah" and "Yuck" mean "over my dead
body" or just "pot ugly, but as long as I don't have to write code
like that myself, I can live with others changing it to that look
afterwards and having to read it in the future"?
-parren

On Sun, Jul 13, 2008 at 6:22 PM, Matt Mackall <mpm at selenic.com> wrote:
>
> 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.
>
>