[MERGE] Less bazaar coding style regressions.

[John Arbash Meinel]

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

Marius Kruger wrote:
| Thats right folks,
|  I think I've successfully written a test which checks for bazaar code
| style regressions in the not-yet-commited changes of the working tree
| being tested.
|             Currently I check all newly modified .py files for:
|             * new trailing white space
|             * new leading tabs
|             * new long lines (give warning only)
|             * no newline at end of files
|
| It already came in usefull while preparing this patch
| ./bzr --no-plugins selftest coding_style
| will help people like me to easily prepare non-style-regressing patches.
|
| I mostly copied code from diff and friends.
|
| regards
| marius
|
| --
| a patch a day
|

This seems like an interesting way to go. I'll probably have to think a
bit more about how it will work with PQM, etc. But first a few comments:

1) It doesn't make sense to run this test for an installed bzr, and
there will be no WorkingTree to open in that case. So I would probably
change the line:

wt = WorkingTree.open(bzr_dir)

To catch the exception, and maybe give a missing feature exception
instead. (This won't fail the test suite, but lets the user know that a
test couldn't be run because of a missing ability.)

2) If you look it TestSource, there are probably better ways to get at
the directory containing bzrlib files. This may not be specifically
relevant due to (1), but certainly "bzr" is often not next to "bzrlib".

3) I wouldn't call the function "internal_diff". Maybe
"check_coding_style()" or something like that.

4) PQM runs the test suite with -Werror, so giving a warnings.warn()
will fail the test suite. I don't know how strict we want to be on 80
char lines. As especially when you start 'optimizing' you end up with
more stuff inline (rather than helper funcs), and then you have a lot
more trouble keeping things under 80 chars. (Unless you start using
short variable names, which makes it harder to understand...)

So line-length is a maybe, IMO.

5) It feels a little odd that:

bzr merge
bzr selftest <= will fail
bzr commit
bzr selftest <= will pass

Though I understand the reasoning.

6) There are some PEP8 violations. You need an extra blank line before
"class TestCodingStyle".

7) I also wonder if this test case should just be put in with the other
test_source.py tests. Just to keep logical things together. This is more
of a question, and not a strict demand, though.


Overall, I think I would like something like this to be included. We
need to hash out some of the details, but I'm overall positive on it.

BB:resubmit

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

iEYEARECAAYFAkiMg+AACgkQJdeBCYSNAAO/4gCgrbB7opLWMYDWnIsfgOqIPAk0
AzgAoNJk0XVku4Z90KCQjuv6GCDrX9EJ
=raXE
-----END PGP SIGNATURE-----