[patch] add pyflakes; clean up cruft

[Martin Pool]

On 16 Jun 2006, Robert Collins <robertc at robertcollins.net> wrote:
> On Fri, 2006-06-16 at 15:10 +1000, Martin Pool wrote:
> > This patch adds make targets to run the 'pyflakes' static checker, and
> > cleans up some things I found by running it.  Seeking +1 or comments.
> ...
> >  - Remove most occurrences of 'from foo import *', which is generally
> >    thought bad style and prevents other checks
> 
> err, I don't think is bad style per se. Its extremely useful in some
> precise circumstances - such as 'from stat import *' or 'from errno
> import *' - where the symbols being import are supplied from a __all__
> statement, and where they make sense in the namespace they are being
> imported into. Specifically, from symbol_versioning import * is useful
> and appropriate, because we cannot demand load symbols needed to compile
> source, only symbols used at runtime.
> 
> I can dig through the patch, but can you be more precise about what
> usage of this you find objectionable?

Happy to.  

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 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.)

The particular ones I changed were:

-from bzrlib.conflicts import *
-from bzrlib.decorators import *
-from bzrlib.progress import *
-from bzrlib.symbol_versioning import *
-from bzrlib.textfile import *
-from bzrlib.tuned_gzip import *
-from stat import *

None of these are outrageous or causing bugs.  Some of them don't have
__all__ and so are a bit naughty: conflicts, progress, textfile.

In most cases we're importing * but only using one or two names: e.g.
tuned_gzip only exports one name; most users of stat want only S_ISDIR.
So just naming the thing is easy.

symbol_versioning requires the most typing and so is just a concession
to pyflakes, but since it did find some real bugs it's a price I'm
willing to pay.

One option would be to rename that module and say

  from bzrlib import deprecated

  @deprecated.method(deprecated.zero_eight)
  def foo():

> >  - Generally remove 'import bzrlib' in favour of importing the
> >    particular submodules that are needed.
> 
> This too - Its nice to know that 'bzrlib' is always available. If we
> have 'import bzrlib.foo' then sure, import bzrlib is redudant and I
> agree removing it is fine.
> 
> generally +1, would like to discuss the two points above a little more
> before its merged though.

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.

-- 
Martin