[MERGE] enhanced mv command

Marius Kruger amanic at gmail.com
Fri Jan 12 07:07:22 GMT 2007


On 1/12/07, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> > Aaron Bentley wrote:
> >     ^^^ I'm dubious about trying to generate correct English plurals for
> >     error strings -- we've always just done "File(s) exist".
> Once you start down this path, internationalization gets trickier, and
> even English plurals aren't entirely regular.  Plus the source is harder
> to read.  So as a rule I try to avoid building grammar into my programs.


I'm 100% with you

>     ^^^ All this does is insert a trailing space.  Not an improvement :-)
> >
> >
> > very funny.
> > I just can't win:  If I set my editor to remove those pesky trailing
> > whitespace
> > automatically you complain that I remove them. if I don't, you complain
> > that I add them.
>
> It is spurious changes that I object to; both adding and removing
> whitespace fall into that category, as does changing the execute bit.
> (changing the names of the errors comes close, but I wouldn't consider
> it *spurious*, per se, just out-of-place in this bundle)


I don't really worry if it is spurious or not, since the code is properly
version controlled nobody could argue who the true owner is or what changed.
What is more important to me is if it is an improvement or a deterioration:
* Removing trailing white space is an improvement as it gets you closer
   to the (unenforced) convention of not having trailing white space in
code.
* Adding trailing white space is obviously a deterioration never mind
spurious.
* Changing BzrError to errors.BzrError is also an improvement, as it gets
you closer
   to the (unenforced) convention.

But I agree that having a bunch of unrelated trivial improvements riddling a
patch
isn't nice and should really be avoided. Thats why I propose doing it in one
big go
and get it done with.

If you don't like trivial improvements like this even if it is done in an
organized fashion,
I think the vcs and other tools are failing you.
* Something which would be useful for me is to have a gui annotate, which
could
   drill down when you click on a line: Say the line's revisoin is 10, do a
'bzr annotate -r9'.
   I.e re-annotate up to a revision before its current last change.

I make a practice of diffing my changes, usually against the mainline,
> so that I can catch and undo spurious changes.


I also started to do that,  I also parse it through
1) add-trailing-white-space-detector,
    (I should probably have a alter-trailing-white-space-detector.)
2) break-up-into-words | aspell list
    (as I'm quite bad at spelling, but theres no tools to fix my grammar)


> I think after this thing is merged I should start looking at doing a
> > patch which:
> > 1) removes all the trailing white space once and for all and
> > 2) introduce a test which checks that nobody violates this ever again.
> >
> > And I might also write a plugin which warns you that you are
> > adding trailing white space when you commit (even failing the commit if
> > you choose).
>
> The last sounds the most appealing to me.  Spurious whitespace changes
> have harmful effects on merging, and I have several outstanding branches
> at the moment, so I would object to 1).

If I write a test without fixing it, the test will fail and thus (I think)
you would
not be able to merge it in, as it will start failing all over.
What I was thinking is also providing a small python script which helps you
remove
all trailing white space in all the .py files.
Then all diverging branches must just apply it to themselves first before
attempting to merge back in. Maybe a special date must be set for it,
for example: "All trailing white space will be removed on 2007-02-12
and tests for this will be added, all bundles submitted after that must be
cleaned up by the branch (using a script provided)."

> I see that some of the latest commits still introduce new trailing white
> > space,
> > when is it going to stop?
>
> Well, it takes time to adjust one's habits.

but its sped up if its enforced
(like speeding-fines helps you to not drive way over the speed limit)

>     +            except OSError, e:
> >     +                self._rollback_move(moved)
> >     +                raise errors.BzrRenameFailedError(entry.from_rel,
> >     entry.to_rel,
> >     +                    e[1])
> >     +            except errors.BzrError, e:
> >     +                self._rollback_move(moved)
> >     +                raise
> >
> >     ^^^ Is it possible to catch more specific errors here?  BzrError and
> >     OSError are very broad.
> >
> > (Again I didn't write this, so don't be too hard on me)
> > I think its ok, as we just want to rollback and raise it again.
> > It is done like that elsewere in the file too.
>
> Really, I don't see why anyone would do this.  Either you want to catch
> *all* exceptions and roll back, or you want to catch specific
> exceptions, and only roll back on those.  In particular, OSError does
> not necessarily indicate that a rename failed.  You would have to wrap
> the try/catch around osutils.rename in order to know whether the rename
> failed.


I don't really know what to do about this.

> === modified file 'bzrlib/builtins.py'
> > -from bzrlib import symbol_versioning
> > +from bzrlib import (symbol_versioning,
> > +                    osutils,)
>
> I think the current import style is
>
> from bzlib import (
>     osutils,
>     symbol_versioning,
>     )
>
> It's alphabetical order, with each item formatted the same, to minimize
> conflicts.  Not a big deal, just for future reference.
>
> The one below is older (and probably my work).
>
> >  from bzrlib.patches import (PatchSyntax,
> >                              PatchConflict,
> >                              MalformedPatchHeader,


It will be a sweet exercise to write tests for these too.
But then it also needs to be cleaned up everywhere.

> === modified file 'bzrlib/tests/__init__.py'
> > --- bzrlib/tests/__init__.py  2007-01-11 17:00:25 +0000
> > +++ bzrlib/tests/__init__.py  2007-01-11 17:31:34 +0000
> > @@ -665,6 +665,16 @@
> >          if not (left is right):
> >              raise AssertionError("%r is not %r." % (left, right))
> >
> > +    def assertNone(self, obj, msg):
> > +        """Fail if obj is not None"""
> > +        if (obj is not None):
> > +            raise AssertionError(msg)
> > +
> > +    def assertNotNone(self, obj, msg):
> > +        """Fail if obj is None"""
> > +        if (obj is None):
> > +            raise AssertionError(msg)
> > +
>
> If we're going to add assert*None, it would make sense to provide a
> default message.


I had that in for both, but after working with it a while, I realized that
a default message is pretty uselese like  "obj is None",
so I'd rather force a proper message, which could cast some light on the
context.

Another option would be to extend assertIs to take a
> message, and add assertIsNot.


Its up to you, but I prefer assertNone.

>      def test_mv_unqualified(self):
> >          self.run_bzr_error(['^bzr: ERROR: missing file argument$'],
> 'mv')
> > -
> > +
>
^^^ spurious whitespace change

My goodness, after all the effort of writing a
add-trailing-white-space-detector,
I still missed some.
I find it almost impossible to get this right.


Anyhow, I'd rather have this in than out, so +1, and I'll fix it up when
> I merge it.
>
Aaron
>

thanks a lot man. especially for "fixing it up when you merge it"

regards
marius



-- 



I code therefore I am.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20070112/e265fbd9/attachment.htm 


More information about the bazaar mailing list