make pep8 test routine reusable for other projects #2486
Conversation
Nothing changes for matplotlib test case but the routine can now be imported and used from another project. Also makes it better readable I think to have all the custom exclusions grouped at the head of the file. I also had to move the currently active manual skip to the bottom.
- among other things, this will make it easy to spot pep8 pollution in pull requests before merging - currently won't work because it depends on changes in matplotlib (see matplotlib/matplotlib#2486), have to wait and see how that turns out
|
In principle this is absolutely fine. The big problem is that the way this is architected, means that nose will pick up this test even though you've skipped it in main. I think the only solution to this currently is to rename the test function to something like assert_pep8, and then add a commented out function called "test_pep8" (or some other nose compatible name) which calls it but is currently commented out. Your code would then call "assert_pep8" rather than the test function itself. How does that sound? |
|
Ok, we don't use nose so I didn't know about that aspect. Hmm.. I think I got a knot in my brain reading your recommendation. How about the following?
EDIT: Oh ok, see what you mean now. Actually its the same as what I was thinking just without moving the logic to |
|
With nose, you can decorate a function as nottest: http://nose.readthedocs.org/en/latest/testing_tools.html#nose.tools.nottest |
|
Actually, after I implemented to reuse this, another team member of our project wants to go for flake8 instead (to have even more warnings... ;)). So, we will probably not reuse the pep8 checker routine from here, after all. So, no need to merge this from my point of view, after all (although I still think this improves readabillity). If you still want to merge this I can use the decorator for the skipping as suggested above.. |
|
I think some good work has gone into this - I'm not convinced that this is the final step to allow people to re-use the PEP8 test of matplotlib, but it is a start. As such, I'm happy enough to merge this now. Thanks @megies. |
Make pep8 test routine reusable for other projects (work in progress).
|
Actually one really can reuse it, I tried and it worked ok (see PR in our project and the failed test case it produced). I would still move the logic (the Thanks for merging. Also, I implemented to automatically except untracked files from the checking in our project (see commit). I would offer to add this in matplotlib too via a PR if you are interested to have it. |
I'd like to reuse the pep8 testing routine from matplotlib in one of our projects (since it's a dependency anyway) rather than duplicate code (and maintenance) by copy/patching it into our codebase. These are the minimal changes necessary for reusability.
Nothing changes for matplotlib test case but the routine can now be imported and used from another project.
Also, it's better readable I think, to have all the custom exclusions grouped at the head of the file.
I also had to move the currently active manual skip to the bottom.
Furthermore, in principle I think the whole logic rather belongs into
/testing/..and should only be imported and called in/tests/.., but I did not want to change this without a go-ahead (and its not that important, I guess).Also, I will include a check if failing files are untracked to exclude them from the results by using a system call to
git(try/excepted ifgitis available of course). If this PR gets a nod, I would offer to implement this right here, otherwise I'll do that in our test setup.