[bug 1101253] Add flake8 check to travis.

[koddsson]

flake8 all the things!

[Osmose]

Thanks for the PR! Two things though:

1. The bug in question isn't about code for the snippets service, it's about snippet code, IE snippet templates or the code for special individual snippets.

2. I am not a fan of making a test suite fail due to style guidelines. There are plenty of good reasons to break PEP8, and it's a bit obnoxious to have to mark each instance just to get your tests to pass.

   I'm a fan of automatically running the style check against PRs though, just not having the test suite fail because of it.

I imagine @glogiotatidis will disagree with me because IIRC he's done this to other projects before. :P

[koddsson]

1. Right, I sometimes miss certain words. We still want style checks for this repo?
2. Hmm I wonder if there is a way to make travis display an error instead of flat out failing. I'll investigate that and modify this PR accordingly.

For what it's worth I'm a huge fan of style checks and for me and my projects and I generally believe that style checks should fail tests. I however do see your point as I imagine that sometimes snippets (and fixes to existing snippets) need to be pushed through before a certain date. Would failing test runs on style check fails for the snippets-service make still sense though since I'm guessing pushing stuff through isn't as important/time sensitive?

[Osmose]

Hmm I wonder if there is a way to make travis display an error instead of flat out failing. I'll investigate that and modify this PR accordingly.

I'd like to wait and see how possible this is before making a decision, as well as waiting for @glogiotatidis 's input. :D

[koddsson]

@Osmose @glogiotatidis

This last commit is also an option but unfortunately travis doesn't be able to give back Style check failed status so this might be the next best thing. We run the linting after the tests and ignore the status of it so we have the lint results but it doesn't block the running of the tests.

[Osmose]

I like it! Seems like most of the errors are line length. Is there any way to set the maximum line length to 100? Alternatively, can we ignore that specific error type?

[koddsson]

@Osmose As you wish :)

[glogiotatidis]

I like respecting pep8 but I understand what @Osmose means when he says that "There are plenty of good reasons to break PEP8". I like what you've done here @koddsson, it's a win-win.

Having a jenkins (or other service) to comment on PRs with pep8 warnings would be ideal, because otherwise you'll have to take action to see if there are any warnings. I used to work on imhotep and flake8 plugin. If that sounds interesting we can revive it.

Thanks @koddsson!

[koddsson]

Having a jenkins (or other service) to comment on PRs with pep8 warnings would be ideal, because otherwise you'll have to take action to see if there are any warnings.

This is just what I was thinking, if you need to go to travis for each build to check the linter nobody is going to realistically gonna do it (unless the owners remember to do so for every PR)

I used to work on imhotep and flake8 plugin. If that sounds interesting we can revive it.

I like it! To me the most ideal situation would have travis comment on the with warnings on PRs much like how coveralls.io does is but that doesn't seem to be an option.

[koddsson]

I was in the shower this morning and thinking to myself that I could probably write some small app in python/bash that I could execute on travis that would comment on the PR with the flake8 results. I'm looking into that possibility now and I might use this PR to test it so it might get noisy in here.

[koddsson]

I actually went on and wrote a little script that does this with a GitHub API token key and a new dummy user @mozilla-travis, but it seems not possible without exposing the token to the world since travis won't let PRs get access to secure variables.

Would that be such a bad idea? (I'm just thinking out loud here). Since @mozilla-travis is just a dummy account with no access to anything would it be so bad if someone had it's token key?

What do you guys think? @Osmose @glogiotatidis

[Osmose]

I don't see any huge issue myself. I'll take a look at the code tomorrow or Monday.

[mozilla-travis]

snippets/urls.py:23:5: E128 continuation line under-indented for visual indent
snippets/urls.py:30:1: E265 block comment should start with '# '
snippets/urls.py:33:9: E128 continuation line under-indented for visual indent
snippets/settings/base.py:4:1: F403 'from funfactory.settings_base import *' used; unable to detect undefined names
snippets/settings/__init__.py:4:1: F403 'from base import *' used; unable to detect undefined names
snippets/settings/__init__.py:8:5: F403 'from travis import *' used; unable to detect undefined names
snippets/settings/__init__.py:11:9: F403 'from local import *' used; unable to detect undefined names
snippets/settings/__init__.py:19:9: F403 'from test import *' used; unable to detect undefined names
snippets/base/managers.py:50:101: E501 line too long (113 > 100 characters)
snippets/settings/travis.py:13:22: E203 whitespace before ':'
snippets/settings/travis.py:14:26: E203 whitespace before ':'
snippets/base/tests/test_forms.py:9:101: E501 line too long (101 > 100 characters)
snippets/base/tests/test_managers.py:208:5: E303 too many blank lines (2)
snippets/base/tests/test_models.py:251:1: W391 blank line at end of file
snippets/base/tests/test_admin.py:88:1: W293 blank line contains whitespace
snippets/base/admin.py:223:37: E713 test for membership should be 'not in'

[koddsson]

Ok, I added the token in cleartext (I can always revoke it), commit that and that triggered a build on travis which triggered @mozilla-travis to comment here with the PEP8 errors.

[Osmose]

Maybe name this TRAVIS_BOT_GITHUB_TOKEN just to be explicit?

[Osmose]

r+wc, the two comments are nits. Really nice job on this, I'll totally be using this in other projects. :D

Let me know when you've updated the PR and I'll merge.

[koddsson]

I'll totally be using this in other projects. :D

I'm glad to hear you say that, I was actually thinking about abstracting this into a pypi package so that other people can use this! After I'm done with that I'll probably come back here and refactor this to use that package.

[koddsson]

Hmm.. Seemed to break after my last commit.

[mozilla-travis]

snippets/urls.py:23:5: E128 continuation line under-indented for visual indent
snippets/urls.py:30:1: E265 block comment should start with '# '
snippets/urls.py:33:9: E128 continuation line under-indented for visual indent
snippets/settings/__init__.py:4:1: F403 'from base import *' used; unable to detect undefined names
snippets/settings/__init__.py:8:5: F403 'from travis import *' used; unable to detect undefined names
snippets/settings/__init__.py:11:9: F403 'from local import *' used; unable to detect undefined names
snippets/settings/__init__.py:19:9: F403 'from test import *' used; unable to detect undefined names
snippets/settings/travis.py:13:22: E203 whitespace before ':'
snippets/settings/travis.py:14:26: E203 whitespace before ':'
snippets/settings/base.py:4:1: F403 'from funfactory.settings_base import *' used; unable to detect undefined names
snippets/base/managers.py:50:101: E501 line too long (113 > 100 characters)
snippets/base/tests/test_forms.py:9:101: E501 line too long (101 > 100 characters)
snippets/base/tests/test_admin.py:88:1: W293 blank line contains whitespace
snippets/base/admin.py:223:37: E713 test for membership should be 'not in'
snippets/base/tests/test_managers.py:208:5: E303 too many blank lines (2)
snippets/base/tests/test_models.py:251:1: W391 blank line at end of file

[koddsson]

Yess! I think we're ready @Osmose!

[Osmose]

Merged in c707e13...8b4bcb0. Great work!