[patch] add pyflakes; clean up cruft
John Arbash Meinel
john at arbash-meinel.com
Fri Jun 16 15:43:50 BST 2006
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
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.
>
> The patch itself is long and generally uninteresting so to summarise:
>
+1 to the general concept of cleaning up the cruft. I have a few
comments on the actual patch, but not much.
Also, I still think we should run the test suite through a coverage
tool. And if we can integrate that into the Makefile, good for us.
...
> +import errno
> +import os
> +from os.path import dirname
> import sys
> -from os.path import dirname
How thorough are you trying to be, because it would seem that directly
importing dirname violates your general guidelines above.
...
> === modified file 'bzrlib/atomicfile.py'
> --- bzrlib/atomicfile.py 2005-12-18 04:08:46 +0000
> +++ bzrlib/atomicfile.py 2006-06-15 06:41:19 +0000
> @@ -14,11 +14,11 @@
> # along with this program; if not, write to the Free Software
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> -
> +import errno
> +import os
Shouldn't you be importing 'socket' here, rather than later?
>
> from warnings import warn
> from bzrlib.osutils import rename
> -import errno
>
> class AtomicFile(object):
> """A file that does an atomic-rename to move into place.
> @@ -35,7 +35,7 @@
> if mode != 'wb' and mode != 'wt':
> raise ValueError("invalid AtomicFile mode %r" % mode)
>
> - import os, socket
> + import socket
> self.tmpfilename = '%s.%d.%s.tmp' % (filename, os.getpid(),
> socket.gethostname())
> self.realfilename = filename
> @@ -30,39 +26,29 @@
> from bzrlib.decorators import needs_read_lock, needs_write_lock
> from bzrlib.delta import compare_trees
> import bzrlib.errors as errors
> -from bzrlib.errors import (BzrError, InvalidRevisionNumber, InvalidRevisionId,
> - NoSuchRevision, HistoryMissing, NotBranchError,
> - DivergedBranches, LockError,
> - UninitializableFormat,
> - UnlistableStore,
> - UnlistableBranch, NoSuchFile, NotVersionedError,
> +from bzrlib.errors import (InvalidRevisionNumber,
> + NotBranchError,
> + DivergedBranches,
> + NoSuchFile,
> NoWorkingTree)
I personally prefer the 'errors.Foo' rather than importing the specific
error at the top. I saw that you fixed some of these, is there a reason
you didn't fix all of them? (errors.py is one of those things that is
very likely to be demand loaded. Everything uses it, but really only
when something goes wrong)
> -import bzrlib.inventory as inventory
> -from bzrlib.inventory import Inventory
> from bzrlib.lockable_files import LockableFiles, TransportLock
> from bzrlib.lockdir import LockDir
> -from bzrlib.osutils import (isdir, quotefn,
> - rename, splitpath, sha_file,
> - file_kind, abspath, normpath, pathjoin,
> - safe_unicode,
> +from bzrlib.osutils import (
> rmtree,
> )
Same thing here, especially with only 1 function, it would seem you
could just switch to calling it by the full path.
...
> @@ -899,7 +887,7 @@
> self._base = self._transport.base
> self._format = _format
> if _control_files is None:
> - raise BzrBadParameterMissing('_control_files')
> + raise ValueError('BzrBranch _control_files is None')
Why did you switch to 'ValueError'. I thought we wanted to avoid raising
Python builtin exceptions as much as possible.
...
> for l in _locs:
> try:
> - return urlutils.join(self.base[:-1],
> + return urlutils.join(self.base[:-1],
> self.control_files.get(l).read().strip('\n'))
> except NoSuchFile:
> pass
These will conflict with my recent fixes to allow compatibility with
local abs paths. But it is easy enough to resolve.
> @deprecated_function(zero_eight)
> -def ScratchBranch(*args, **kwargs):
> - """See bzrlib.bzrdir.ScratchDir."""
> - d = ScratchDir(*args, **kwargs)
> - return d.open_branch()
> -
> -
> - at deprecated_function(zero_eight)
> def is_control_file(*args, **kwargs):
> """See bzrlib.workingtree.is_control_file."""
> return bzrlib.workingtree.is_control_file(*args, **kwargs)
Yay! goodbye ScratchBranch.
Shouldn't deprecated_function take a string parameter which could tell
you what to use instead of the deprecated function? It seems like it
would be a lot more helpful if you could point people in the right
direction when you deprecate something.
...
> +from bzrlib.branch import Branch, BranchReferenceFormat
> +from bzrlib import (branch, bzrdir, errors, osutils, ui, config, user_encoding,
> + repository, log)
One thing I should mention. The above is probably the best route to go.
In testing demandload can't do 'import foo.bar as bar'. It does support
'from foo import bar'. Though I seemed to have some problems with it
auto-importing a module. I know you can get errors because of variable
hiding (if there is a variable 'bar' in module foo, it won't import the
module 'bar' sitting next to it)
...
> @@ -1412,7 +1403,7 @@
> """List unknown files."""
> @display_command
> def run(self):
> - from bzrlib.osutils import quotefn
> + from osutils import quotefn
> for f in WorkingTree.open_containing(u'.')[0].unknowns():
> self.outf.write(quotefn(f) + '\n')
This looks very very wrong.
I would be okay either leaving it alone (as bzrlib.osutils import) or using
quotefn = osutils.quotefn
But I thought we were trying very hard to always use absolute imports.
(Especially since I thought newer version of python are going to require
them)
...
> if file:
> - message = codecs.open(file, 'rt', bzrlib.user_encoding).read()
> + message = codecs.open(file, 'rt', user_encoding).read()
>
You should not directly import variables. Because it prevents us from
changing it in one central location and having the rest of the library
recognize it. That is why we never do 'from bzrlib.ui import
ui_factory', etc.
user_encoding seems extremely likely to change as part of the test
suite. (In fact, I'm sure I play with it, though this code path isn't
very well tested, since it wants to spawn an editor.).
...
> +class BadFileKindError(BzrNewError):
> + """Cannot operate on %(filename)s of unsupported kind %(kind)s"""
> +
> +
> +class ForbiddenFileError(BzrNewError):
> + """Cannot operate on %(filename)s because it is a control file"""
Shouldn't this be ForbiddenControlFileError? I'm thinking we need
something to raise if someone asks to control a socket or a badly formed
unicode path.
...
> @@ -465,7 +463,7 @@
> """
> result = Graph()
> if not revision_ids:
> - pending = set(self.all_revision_ids())
> + pending = set(self._all_revision_ids())
> required = set([])
> else:
> pending = set(revision_ids)
> @@ -624,7 +622,7 @@
> return self._check(revision_ids)
>
> def _check(self, revision_ids):
> - result = bzrlib.check.Check(self)
> + result = check.Check(self)
> result.check()
> return result
>
> @@ -861,7 +859,7 @@
> vf = self._get_revision_vf()
> versions = set(vf.versions())
> if not revision_ids:
> - pending = set(self.all_revision_ids())
> + pending = set(self._all_revision_ids())
> required = set([])
> else:
> pending = set(revision_ids)
Are these valid fixes for all_revision_ids => _all_revision_ids. The
problem is the function that calls them
(get_revision_graph_with_ghosts()) doesn't really have a way to
implement them without using all_revision_ids() if you don't supply it
with a target. Which is why in my other fixes I actually promoted that
call path to be deprecated as well. Just hiding the deprecation warning
leaves you no better than before when you were directly calling the
function. It still is a required function rather than being optional.
...
> -# (C) 2005, 2006 Canonical Ltd
> +# Copyright (C) 2005, 2006 Canonical Ltd
>
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
> @@ -19,23 +19,18 @@
> import os
> import sys
If you are going to clean up the copyright, how about cleaning up the
blank spaces while you're at it?
...
> === modified file 'bzrlib/tests/test_commit_merge.py'
> --- bzrlib/tests/test_commit_merge.py 2006-02-20 09:50:27 +0000
> +++ bzrlib/tests/test_commit_merge.py 2006-06-16 03:42:36 +0000
> @@ -1,4 +1,4 @@
> -# Copyright (C) 2005 by Canonical Ltd
> +# Copyright (C) 2005, 2006 by Canonical Ltd
>
> # This program is free software; you can redistribute it and/or modify
> # it under the terms of the GNU General Public License as published by
Same copyright comment.
...
> if __name__ == '__main__':
> import sys
> - if '--profile' in sys.argv:
> - args = sys.argv[:]
> - args.remove('--profile')
> - sys.exit(profile_main(args))
> - elif '--lsprof' in sys.argv:
> - args = sys.argv[:]
> - args.remove('--lsprof')
> - sys.exit(lsprofile_main(args))
> - else:
> - sys.exit(main(sys.argv))
> + sys.exit(main(sys.argv))
Since you moved the commands to be exposed in the main 'bzr', shouldn't
all this stuff be removed?
Overall, I'm really happy to see some cleanup. I think the code could
use several passes of this sort of thing. The only big thing was the
direct access of 'user_encoding'.
The rest is all minor.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFEksOmJdeBCYSNAAMRAsKqAJ41ik7PHq10uTwlNpxQhoWd/CFdYgCgsFiY
yfRhoIQMTdo8D/Sx/2tWkb0=
=ad20
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list