[patch] add pyflakes; clean up cruft
Martin Pool
mbp at canonical.com
Tue Jun 20 05:45:49 BST 2006
On 16 Jun 2006, John Arbash Meinel <john at arbash-meinel.com> wrote:
> +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.
Yes I agree (though to me that's a separate issue from pyflakes).
> > +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.
Not totally thorough, on the first pass I just wanted to fix things that
really stuck out and put that up for review. I may do another one when
this is in.
> > === 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?
Again I didn't clean up every instance, and in particular I thought that
Robert was going to remove the socket dependency entirely.
> > @@ -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)
Yes, again just a principle of minimal changes.
> > -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.
Thanks, fixed, plus some similar things.
> ...
>
> > @@ -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.
In general yes, but I think this one can be justified. We should raise
our own exceptions because callers might want to catch and distinguish
them, and secondarily because many builtin error classes produce poor
messages. In this case however we're basically trying to catch a bug in
the caller and that's what ValueError normally represents.
>
>
> ...
>
> > 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.
That would be nice; in general functions that want to helpful like this just
raise the warning themselves rather than using the deprecator decorator.
> > +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)
Yes, it seems to respect DontRepeatYourself more too.
> > @@ -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)
Yes, good catch, the import should just be deleted and we'll use the
qualified name.
> > 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.).
A very good point.
> > +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.
Yes, that would be better, though a unix socket would be covered by BadFileKindError.
> > @@ -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.
We should probably just remove the deprecation for now.
> If you are going to clean up the copyright, how about cleaning up the
> blank spaces while you're at it?
I'll do that in another pass.
> > 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?
I'll clean them up too.
Thanks so much for the detailed review.
--
Martin
More information about the bazaar
mailing list