[MERGE] enhanced mv command

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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 + ": "
> +

...

>          tree.add(['test.txt'])
>  
>          self.run_bzr_error(
> -            ["^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
versioned.


>          
>          self.run_bzr_error(
> @@ -171,7 +171,7 @@
>      
>          os.remove('a')
>          self.run_bzr_error(
> -            ["^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.
or maybe
bzr: ERROR: Cannot rename %s => %s. Target is already versioned.

...

>          self.run_bzr_error(
> -            ["^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
being used.

...

> === 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
"errors.NotADirectory".
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,
>                             BzrError,
>                             ConflictFormatError,
> -                           WeaveRevisionNotPresent,
> +                           FilesExist,                     
> +                           NotADirectory,
>                             NotBranchError,
> +                           NotInWorkingDirectory,
>                             NoSuchFile,
>                             NotVersionedError,
>                             MergeModifiedFormatError,
>                             UnsupportedOperation,
> +                           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:

try:
  wt.rename_one()
except BzrError:
  ...

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

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


John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFivlpJdeBCYSNAAMRAsKpAKC8QzCZfiyneRdjZPC4W6QAZKuqmACfa6Mk
oNROrlx4TBeuXUbwqp7OSV0=
=w6au
-----END PGP SIGNATURE-----