[MERGE] Less bazaar coding style regressions.

[Marius Kruger]

finally I got a chance to look at this again.

2008/7/27 John Arbash Meinel <john at arbash-meinel.com>

> 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.
>
> 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.)


I saw TestSource.get_bzrlib_dir raises TestSkipped('Cannot find bzrlib
source directory
which sounded similar to our situation. (I also saw that for
UnavailableFeature
I need to make a Feature, which seems a bit of overkill)
So I also opted for TestSkipped, but I can change it if you like.

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".


Since I'm looking for the bazaar branch, I'm not really interested in
bzrlib.
AFAIUI when you are running tests you're supposed to use the executable,
in the bazaar branch you are trying to test. So I figured that trying to use
the
directory containing the bzr executable being executed would be appropriate.


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


ok changed to check_coding_style, and added a comment explaining that
it is a text_differ suitable to be passed to diff.DiffText


> 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.


I didn't expect PQM to run the tests with -Werror. This doesn't really make
sense to me.
Are there no way to emit warnings by tests?
I would still like to at least warn people running the tests
that they are adding long lines, so I changed it to just print the message.
hope thats ok.


> 5) It feels a little odd that:
>
> bzr merge
> bzr selftest <= will fail
> bzr commit
> bzr selftest <= will pass
>
> Though I understand the reasoning.


The point of the test is for PQM to not allow in more trailing white space.
AFAIUI, PQM does a merge and then run the tests, which will then refuse to
let in new trailing white space.

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

(this is not applicable any more after the code move suggested in 7))


> 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.


I didn't know test_source existed.
I moved my test there now.

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


thanks for looking at this

regards
marius
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20081208/b1b2cde5/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: no_new_trailing_white_space.patch
Type: text/x-diff
Size: 12842 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20081208/b1b2cde5/attachment-0001.bin