Repository navigation
Include PEP8 plugin in test framework #58
Description
It was suggested by @wkerzendorf that we may want to have a convenient way to test pep8 compliance within our test framework. There is a py.test plugin for this here:
It does seem configurable so we can turn off some of the more annoying false-positive warnings off if necessary.
Activity
embray commentedon Nov 8, 2011
+1
eteq commentedon Nov 9, 2011
@jiffyclub - do you think you can take a crack at this (in particular, adding it as an option to the test function)? Also useful might be http://pypi.python.org/pypi/pytest-cov which runs coverage tests.
jiffyclub commentedon Nov 9, 2011
I'll see what can be done as soon as I get a chance.
jiffyclub commentedon Nov 9, 2011
The py.test pep8 checker just uses the pep8 package: http://pypi.python.org/pypi/pep8, so getting this into astropy would mean packaging the py.test pep8 plugin, and the pep8 package itself. It looks like they are both single file modules, so that's not bad, but there could be import issues since the py.test plugin imports pep8. The packaged plugin could theoretically be modified to import a local copy of pep8. This seems a little redundant when you can just use the pep8 script itself.
Similarly, the py.test coverage plugin requires the coverage package: http://pypi.python.org/pypi/coverage, which is not a simple, single file module. I don't think it would be in any way practical to include this.
So, given that these py.test plugins are not self-contained do we want to go ahead and try to include them (pep8, anyway)?
mdboom commentedon Nov 9, 2011
Oops. Didn't mean to push the close button ;)
embray commentedon Nov 9, 2011
Yeah, github makes it way too easy to do that accidentally. There should at least be a confirmation dialog or something.
eteq commentedon Nov 9, 2011
Hmm... I see four approaches, then:
pip install pytest-pep8to make this work"I have a slight preference for 1, but 2 is ok also.
astrofrog commentedon Nov 10, 2011
I like option 1. It's something that will be useful to such a small handful of users that I don't think we need to clutter up the repository will all the dependencies required for it to work. Checking pep8 compliance should therefore not run by default when doing
python setup.py testbut should have a special switch, e.g.python setup.py test --pep8or a separatepython setup.py test-pep8.mdboom commentedon Nov 10, 2011
+1 for option #1. There's two use cases here: 1) developers and 2) users. 1) cares about this stuff, 2) just wants to make sure their build/installation is sane.
wkerzendorf commentedon Nov 10, 2011
+1 for option 1 as well. I think it would be nice to mention in the development workflow: 'run pep8 before pull-request'
astrofrog commentedon Nov 17, 2011
Can we close this issue since the pull request has been merged?
eteq commentedon Nov 17, 2011
Huh... odd - it was supposed to be closed when I entered a commit message that said "closes #51", but apparently somehow that part did't make it into the message. But anyway, yes, it's supposed to be closed.
Merge pull request astropy#58 from embray/issue-2749
Merge pull request astropy#58 from Cadair/pre-pr
Merge pull request astropy#58 from mdboom/hidpi