[MERGE] Less bazaar coding style regressions.
John Arbash Meinel
john at arbash-meinel.com
Sun Jul 27 15:19:13 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
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.
| 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 selftest <= will fail
bzr selftest <= will pass
Though I understand the reasoning.
6) There are some PEP8 violations. You need an extra blank line before
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.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
More information about the bazaar