[patch] add pyflakes; clean up cruft

[Andrew Bennetts]

Martin Pool wrote:
[...]
> My main motivation here wasn't that they were wrong or gross but rather
> to scratch pyflakes's back in the hope that it would scratch mine - if
> you have 'import *' it disables some checks.  (This is a pyflakes bug -- 
> I don't see any conceptual reason why they couldn't look at the imported
> module and see what symbols will come in -- but we have to work with the
> tools we have.)  Removing them makes pyflakes happy and doesn't
> particularly disrupt our source.

I guess strictly speaking it is a pyflakes bug, but a hard one to fix 100%
correctly.

The basic problem is that python does almost everything at runtime, including
imports and definitions, and so pyflakes can't cover all cases because it
doesn't actually execute the code, just inspect the compiled code.

For instance, how can pyflakes find the imported module?  Currently you can do
e.g. "cd /path/to/bzrlib/transport; pyflakes sftp.py" and it will give the same
results as running pyflakes on that file from any other location.  pyflakes has
no need (at the moment) to know that that sftp.py is bzrlib.transport.sftp,
where the bzrlib package is rooted at /path/to/bzrlib.  The possibility that
import hooks might be involved make this even harder.

Also, say you do "from other_module import *", and other_module contains this
code:

    def make_constants():
        consts = ['FOO', 'BAR']
        for const in consts:
            globals()[const] = const
        # ...
        __all__.extend(consts)
    make_constants()

(This isn't purely hypothetical, I've seen variations on this idea as a way to
define constants, the motivation is DRY).

It would be very hard for pyflakes to know that FOO was meant to be a symbol
that "from other_module import *" imported.  It could in principle do it by
actually executing the code, but not executing code is one of the features of
pyflakes...

It would be nice if there were ways to explicitly give pyflakes hints so that it
could overcome some of these situations (e.g. something like
"--python-path=/path/to/bzrlib" to help with import guesswork).  And of course
tricks like "make_constants" need to be balanced against their unfriendliness to
static analysis tools (including API documentation generators, ctags, and
grep!).

I guess the trick is to know when to make the tools smarter, and when to make
your source more accessible to the tools.

> (I wanted to see how much effort or distortion would be necessary to
> make things pyflakes-clean; this is one case but it's not unreasonable.)

Avoiding "import *" also has a slight benefit for human readers -- they don't
need to hunt around to find out where a symbol comes from.  It's always
explicitly declared in the file that uses it.  (And it's much harder to
accidentally import two different things with the same name without noticing,
especially because pyflakes will notice even if you don't).

[This reminds me a little of a point made in
http://www.joelonsoftware.com/articles/Wrong.html.  As an example, Joel
describes a coding convention for a particular situation that has the nice
effect that "Every line of code can be inspected by itself, and if every line of
code is correct, the entire body of code is correct."  It's a much weaker
benefit, but explicitly declaring where symbols are imported from makes a code
reviewer's job slightly easier, because it also gives more information more
locally to the code you're reading.  With an appropriate code browser (e.g.
ctags + vim/emacs) this probably isn't worth much.]

[...]
> Yes, in all these cases we have 'import bzrlib.foo', which is why it
> complained about also importing bzrlib.
> 
> I don't really like the idea of referring to 'bzrlib.foo.Foo' without
> importing foo and just relying on the fact that someone else already
> imported it.  It may break and being explicit doesn't cost much.

This is a particularly nasty sort of bug.  Typically running test suites import
everything before a particular test is run (perhaps not true for tests early in
the test run, but in general most tests will be run with most or all of the code
already imported), so it's easy for tests to pass even though there's a missing
import of "bzrlib.foo".  So it's possible to refactor, see that the tests pass,
commit, and later find you broke something.

Well, it's not so nasty because typically it's easy to diagnose and fix this
sort of bug, but it's still embarrassing if it makes it into a release :)

I guess the moral here is that global state shared between tests, even
sys.modules, is dangerous...

-Andrew.