finally I got a chance to look at this again.<br><br>2008/7/27 John Arbash Meinel <span dir="ltr"><<a href="mailto:john@arbash-meinel.com">john@arbash-meinel.com</a>></span><br><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div class="Wj3C7c">Marius Kruger wrote:<br>
| Thats right folks,<br>
| I think I've successfully written a test which checks for bazaar code<br>
| style regressions in the not-yet-commited changes of the working tree<br>
| being tested.<br>
| Currently I check all newly modified .py files for:<br>
| * new trailing white space<br>
| * new leading tabs<br>
| * new long lines (give warning only)<br>
| * no newline at end of files<br>
|<br>
| It already came in usefull while preparing this patch<br>
| ./bzr --no-plugins selftest coding_style<br>
| will help people like me to easily prepare non-style-regressing patches.<br>
|<br>
| I mostly copied code from diff and friends.<br><br>
</div></div>This seems like an interesting way to go. I'll probably have to think a<br>
bit more about how it will work with PQM, etc. But first a few comments:<br>
<br>
1) It doesn't make sense to run this test for an installed bzr, and<br>
there will be no WorkingTree to open in that case. So I would probably<br>
change the line:<br>
<br>
wt = WorkingTree.open(bzr_dir)<br>
<br>
To catch the exception, and maybe give a missing feature exception<br>
instead. (This won't fail the test suite, but lets the user know that a<br>
test couldn't be run because of a missing ability.)</blockquote><div><br>I saw TestSource.get_bzrlib_dir raises TestSkipped('Cannot find bzrlib source directory<br>which sounded similar to our situation. (I also saw that for UnavailableFeature<br>
I need to make a Feature, which seems a bit of overkill)<br>So I also opted for TestSkipped, but I can change it if you like.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
2) If you look it TestSource, there are probably better ways to get at<br>
the directory containing bzrlib files. This may not be specifically<br>
relevant due to (1), but certainly "bzr" is often not next to "bzrlib".</blockquote><div><br>Since I'm looking for the bazaar branch, I'm not really interested in bzrlib.<br>AFAIUI when you are running tests you're supposed to use the executable,<br>
in the bazaar branch you are trying to test. So I figured that trying to use the<br>directory containing the bzr executable being executed would be appropriate.<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
3) I wouldn't call the function "internal_diff". Maybe<br>
"check_coding_style()" or something like that.</blockquote><div> </div><div>ok changed to check_coding_style, and added a comment explaining that <br>it is a text_differ suitable to be passed to diff.DiffText<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">4) PQM runs the test suite with -Werror, so giving a warnings.warn()<br>
will fail the test suite. I don't know how strict we want to be on 80<br>
char lines. As especially when you start 'optimizing' you end up with<br>
more stuff inline (rather than helper funcs), and then you have a lot<br>
more trouble keeping things under 80 chars. (Unless you start using<br>
short variable names, which makes it harder to understand...)<br>
<br>
So line-length is a maybe, IMO.</blockquote><div><br>I didn't expect PQM to run the tests with -Werror. This doesn't really make sense to me.<br>Are there no way to emit warnings by tests?<br>I would still like to at least warn people running the tests<br>
that they are adding long lines, so I changed it to just print the message.<br>hope thats ok.<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
5) It feels a little odd that:<br>
<br>
bzr merge<br>
bzr selftest <= will fail<br>
bzr commit<br>
bzr selftest <= will pass<br>
<br>
Though I understand the reasoning.</blockquote><div><br>The point of the test is for PQM to not allow in more trailing white space.<br>AFAIUI, PQM does a merge and then run the tests, which will then refuse to let in new trailing white space.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">6) There are some PEP8 violations. You need an extra blank line before<br>
"class TestCodingStyle".</blockquote><div>(this is not applicable any more after the code move suggested in 7))<br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
7) I also wonder if this test case should just be put in with the other<br>
test_source.py tests. Just to keep logical things together. This is more<br>
of a question, and not a strict demand, though.</blockquote><div> </div><div>I didn't know test_source existed. <br>I moved my test there now.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Overall, I think I would like something like this to be included. We<br>
need to hash out some of the details, but I'm overall positive on it.<br>
<br>
BB:resubmit</blockquote><div> </div><div>thanks for looking at this<br><br></div></div>regards<br>marius<br>