Travis CI: Upgrade to production Py37 and run flake8 #125
Conversation
|
Gotta love coverage! Adding tests in a non-code file lowers your score. |
|
I've merged half this branch, and added Python 3.7 support to Travis-CI (and removed Python 2.7 and 3.4 while I was at it). I also added MyPy, but not yet flake8. |
|
Added flake8 as dependency and fixed the things it reported in f2c3b4f |
|
Unsure how to resolve conflicts. Both pyenv and poetry or just one of them? |
|
Only Poetry. I've never used pyenv. Did you mean pipenv? I kicked that out today in favour of Poetry. The config for flask8 can be put in the |
|
Ready for review. |
sybrenstuvel
left a comment
sybrenstuvel
left a comment
There was a problem hiding this comment.
Thanks for the cleanup. I still have quite a few questions, though.
| @@ -1,29 +1,19 @@ | |||
| dist: bionic | |||
| language: python | |||
| dist: bionic # required for Python >= 3.7 | |||
There was a problem hiding this comment.
Why remove the comment and move the line? I added the comment for a reason.
There was a problem hiding this comment.
That comment was important when trusty was the default distro on Travis CI because trusty did not support Python 3.7.
But now that xenial is the default distro, the comment is no longer needed because both xenial and bionic fully support Python 3.7 https://blog.travis-ci.com/2019-04-15-xenial-default-build-environment
There was a problem hiding this comment.
Reasoning like this should be part of the commit message.
| cache: pip | ||
|
|
||
| # Environment changes have to be manually synced with 'tox.ini'. | ||
| # See: https://github.com/travis-ci/travis-ci/issues/3024 |
There was a problem hiding this comment.
Why is this comment removed? Again, it was there for a reason.
There was a problem hiding this comment.
That's irrelevant. Both files still list the same environments, and should still be kept in sync. If that's no longer necessary, only one of the files should have the list of environments, not both. Either properly handle this (in a separate commit, as it has nothing to do with getting flake8 to run in CI) or don't touch it.
.travis.yml
Outdated
| #- python: pypy3.5 | ||
| # dist: xenial # Bionic has no Python 3.5 | ||
|
|
||
| before_install: pip install --upgrade pip |
There was a problem hiding this comment.
Because Travis provides an older version of pip and pip is going thru rapid innovation these daze. There is hope that pip will add a real dependency resolver in the near term.
There was a problem hiding this comment.
Would be good to add a comment to this effect, including which version of Pip is now installed by default.
.travis.yml
Outdated
| after_success: | ||
| - poetry run coveralls | ||
| before_script: flake8 . | ||
| script: poetry run py.test tests/ |
There was a problem hiding this comment.
The script and after_success sections haven't changed, you just moved it to one line. Please only include actual changes in the commit, and leave cleanups for another PR. This only makes the diff harder to read.
There was a problem hiding this comment.
It is more important that the file is concise and easier to read than the diff. The diff will be read once, but the file hopefully many times.
There was a problem hiding this comment.
I disagree with that. Cleanups should go in a separate commit. Diffs are definitely read more than once.
.travis.yml
Outdated
|
|
||
| after_success: | ||
| - poetry run coveralls | ||
| before_script: flake8 . |
There was a problem hiding this comment.
Is flake8 installed by default? Personally I'd rather run it with poetry run flake8 ., so that it's running the same version in Travis-CI as you'd get when running locally. Makes things a bit easier to control.
There was a problem hiding this comment.
poetry is installing flake8.
If poetry is also going to run flake8 then this line can be removed. I am not a user of poetry (yet).
There was a problem hiding this comment.
But without poetry run in front of it, it's not going to magically run from the Poetry-managed virtualenv.
| max-line-length = 100 | ||
| max-complexity = 10 | ||
|
|
||
| [pep8] |
There was a problem hiding this comment.
Do other tools that inspect the [pep8] section all have a fallback to the [flake8] section?
There was a problem hiding this comment.
"pep8" was changed to "pycodestyle" several years ago at the BDFL's request. flake8 is a superset of pycodestyle so it does everything pycodestyle did and much more besides.
There was a problem hiding this comment.
Even then, such cleanups should go in a different commit. Getting flake8 to run in CI has little to do with removing unused bits of config.
3 participants
$ flake8 . --count --select=E901,E999,F821,F822,F823 --show-source --statistics
E901,E999,F821,F822,F823 are the "showstopper" flake8 issues that can halt the runtime with a SyntaxError, NameError, etc. Most other flake8 issues are merely "style violations" -- useful for readability but they do not effect runtime safety.
namenamein__all__