[MERGE] enhanced mv command
John Arbash Meinel
john at arbash-meinel.com
Thu Dec 21 21:15:21 GMT 2006
-----BEGIN PGP SIGNED MESSAGE-----
Marius Kruger wrote:
> +class FilesExist(PathError):
> + _fmt = "File(s) exist: %(paths_as_string)s%(extra)s"
> + # used when reporting that files do exist
^- # used when... should really just be a doc string.
v- Do we actually want to quote the paths? We can, but I probably have a
slight preference for *not* quoting.
> + def __init__(self, paths, extra=None):
> + # circular import
> + from bzrlib.osutils import quotefn
> + BzrError.__init__(self)
> + self.paths = paths
> + self.paths_as_string = ' '.join([quotefn(p) for p in paths])
> + if extra:
> + self.extra = ': ' + str(extra)
> + else:
> + self.extra = ''
- -- You want an extra space here
> +class NotADirectory(PathError):
> + _fmt = "%(path)r is not a directory %(extra)s"
> +class NotInWorkingDirectory(PathError):
> + _fmt = "%(path)r is not a the working directory %(extra)s"
> class DirectoryNotEmpty(PathError):
> _fmt = "Directory not empty: %(path)r%(extra)s"
> @@ -491,17 +518,50 @@
> self.repo_format = repo_format
v- We switch to '_fmt' so that we could use the doc-strings as standard
documentation. So use """ instead of #. I realize that isn't the pattern
that is already there, but it is what we are switching to.
> +class AlreadyVersionedError(BzrError):
> + #Used when a path is expected not to be versioned, but it is.
> + _fmt = "%(contextInfo)s%(path)s is already versioned"
> + def __init__(self, path, contextInfo=None):
> + """Construct a new NotVersionedError.
> + :param path: This is the path which is versioned,
> + which should be in a user friendly form.
> + :param contextInfo: If given, this is information about the context,
> + which could explain why this is expected to not be versioned.
> + """
> + BzrError.__init__(self)
> + self.path = path
> + if contextInfo is None:
> + self.contextInfo = ''
> + else:
> + self.contextInfo = contextInfo + ": "
> - ["^bzr: ERROR: destination .* is not a versioned directory$"],
> + ["^bzr: ERROR: Invalid move destination: .* is not versioned$"],
> 'mv', 'test.txt', 'sub1')
^- If this is describing the parent, it probably would be better to say:
"bzr: ERROR: Could not move file. Parent directory .* is not versioned."
I might also prefer to say:
bzr: ERROR: Could not move file %s => %s. Parent directory %s is not
> @@ -171,7 +171,7 @@
> - ["^bzr: ERROR: can't rename: new name .* is already versioned$"],
> + ["^bzr: ERROR: Invalid move destination: .* is already versioned$"],
bzr: ERROR: Cannot rename %s => %s. %s is already versioned.
bzr: ERROR: Cannot rename %s => %s. Target is already versioned.
> - ["^bzr: ERROR: can't rename: both, old name .* and new name .*"
> - " exist. Use option '--after' to force rename."],
> + ["^bzr: ERROR: File\(s\) exist: .+ .+: can't rename."
> + " Use option '--after' to force rename."],
^- You shouldn't need "File(s)" it should just be Files, since you know
there are 2 files. Though it makes me wonder if the right exception is
> === modified file 'bzrlib/workingtree.py'
> --- bzrlib/workingtree.py 2006-12-21 05:11:49 +0000
> +++ bzrlib/workingtree.py 2006-12-21 19:14:35 +0000
> @@ -72,15 +72,19 @@
v- We have been trying to switch away from directly importing the
errors, to importing the 'errors.py' module, and then using
But it is still probably good to keep this in sorted order :)
> from bzrlib import symbol_versioning
> from bzrlib.decorators import needs_read_lock, needs_write_lock
> -from bzrlib.errors import (BzrCheckError,
> +from bzrlib.errors import (AlreadyVersionedError,
> + BzrCheckError,
> - WeaveRevisionNotPresent,
> + FilesExist,
> + NotADirectory,
> + NotInWorkingDirectory,
> + WeaveRevisionNotPresent,
One more point of clarification, as we move forward, we want avoid
raising BzrError by itself. Because it is sort of a catch-all, and we
shouldn't be encouraging people to directly catch BzrError.
And the reason we consider this change to be API compatible, is because
if there was code that did:
it would continue to work if you switch BzrError with a child of BzrError.
v- These both seem very good, though I wouldn't break 'extra=' from its
> if not isdir(to_abs):
> - raise BzrError("destination %r is not a directory" % to_abs)
> + raise NotADirectory(to_abs, extra="Invalid move destination")
> if not self.has_filename(to_dir):
> - raise BzrError("destination %r not in working directory" % to_abs)
> + raise NotInWorkingDirectory(to_dir, extra=
> + "(Invalid move destination)")
> to_dir_id = inv.path2id(to_dir)
> if to_dir_id is None:
> - raise BzrError("destination %r is not a versioned"
> - " directory" % to_dir)
> + raise NotVersionedError(path=str(to_dir),
> + contextInfo="Invalid move destination")
^- I missed this earlier, but we don't use camelCase for parameters. We
use under_score_names. So this should be
context_info="Invalid move destination"
This is used in several places after this point.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the bazaar