test_source harmful?

[Robert Collins]

On Wed, 2009-02-25 at 11:37 +0200, Marius Kruger wrote:
> 2009/2/25 Robert Collins <robert.collins at canonical.com>:
> >> > The whitespace checks are really frustrating me;
> I suspected that this will happen. sorry.
>
> >> I thought that was the point: to put the pain of fixing whitespace
> >> mistakes squarely where they belong, at the point of the person
> >> attempting to introduce them.
> yes, don't allow new ones to be added.

Or we could just not worry.

> > I don't think frustrating people was the point; honestly we 'never' (or
> > extremely rarely) had merge conflicts due to whitespace fixes being
> > done. Creating pain to solve a non-problem is hard for me to understand.
> sorry.

Sorry doesn't remove the friction. Removing the test will remove it.

> > I don't think we can expect other projects to use a source code check
> > like we have done by default. If whitespace conflicts are such an issue
> > we should make our merge handle whitespace conflicts better - because
> > thats what our users will need.
> 
> YES PLEASE!
> Our policy at work is to not have trailing whitespace because it is
> too difficult to
> see what really happend or merge when somebody went crazy with
> whitespace (lots of accidental changes; most programmers don't check
> their diffs before committing)

Sound like you agree: we should not have the whitespace check in
test_source, instead we should deal with any small issues that happen -
and file bugs to provide feedback on where bzr needs to improve.

> >> > I don't think we suffered anywhere near as much when occasional
> >> > trailing whitespace existed as we do when merges are rejected [after
> >> > a long test process]
> >>
> >> Perhaps the checks on source should be done before running any tests
> >> of functionality. Is that sequencing possible?
> >
> > It is,
> pqm could do:
> bzr selftest --first test_source
> 
> > but it still means a round trip [context switching etc].
> 
> thats why I made the bzr-text-checker plugin to warn you about it at
> commit time, or prevent you from committing it in the first place.
> https://launchpad.net/bzr-text-checker
> Content filtering should be able to remove it automatically for you in
> the future.

This ignores the issue: Trailing whitespace is not structurally bad; it
doesn't cause bugs; it might _rarely_ cause merge conflicts. Checking
that something unimportant is set to a particular value is still
checking something unimportant.

> >> > I'd like to delete the test for source code formatting; unlike
> >> > asserts trailing whitespace is not itself a bug.
> well if we don't catch them at test time, trailing whitespace will get added.
> I agree that you don't want to figure this out after a long test run,
> but this will  train you to check it beforehand with `bzr cdiff
> --check-style` or running the test on your side or something.

I don't want to be trained to jump though an unnecessary hoop: I want to
remove the hoop.

Over several years of bzr development, maybe 10-15 minutes lost due to
whitespace conflicts. Over the time since the whitespace check was added
- at least that much already.

> >> Isn't the solution simply to configure your text editor to remove
> >> trailing whitespace before you save the file?
> >
> > Thats one approach.
> in order to do that, you have to know all the source does not have
> trailing whitespace to start off with,
> otherwise you get lots of 'spurious whitespace' changes when editing a
> file. That is the point of this test:
> to guarantee that there is no trailing whitespace in the code, so that
> you can easily use your
> editor to manage it for you.

But trailing whitespace doesn't matter in terms of quality any more than
too much or too little VWS matters. We don't have tests for those things
because they don't matter as far as the test suite is concerned. The
test suite exists to serve us in developing robust high quality code for
our users.

> sorry for the pain, but I think its worthwhile for bzr to figure out
> how to solve this sort of problems,
> which would be useful for the users in the end (who might need it for
> other projects too).

The pain isn't helping sort out the problem; its avoiding sorting out
the problem (which is a merging/tracking issue).

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090227/86969354/attachment.pgp