-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Python 3.6 test environment #2094
Add Python 3.6 test environment #2094
Conversation
* Add py36 environment to tox * Unify dependencies across manual installation and tox * Mention tox in the docs
2f73cfb
to
5ca19ee
Compare
There was no test case checking the behaviour of what happens when trying to {filename} an unknown file. Also changed the braces to `'` chars that we use elseware.
5ca19ee
to
22208a9
Compare
@getpelican/reviewers go for review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the doc text style tweaks I suggested, this looks good to me. Anyone else care to review?
docs/contribute.rst
Outdated
@@ -47,13 +47,16 @@ Or using ``pip``:: | |||
|
|||
$ pip install -e . | |||
|
|||
To conveniently test vs multiple python versions we also provide a tox file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To conveniently test on multiple Python versions, we also provide a
.tox
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me overall; I like how the requirement files are now split up. 👍
.travis.yml
Outdated
include: | ||
- python: 3.6 | ||
env: | ||
- TOX_ENV=py36 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be better to keep indentation here consistent with the rest of the file (2 spaces).
@@ -37,8 +37,7 @@ import-order-style = cryptography | |||
[testenv:flake8] | |||
basepython = python2.7 | |||
deps = | |||
flake8 <= 2.4.1 | |||
git+https://github.com/public/flake8-import-order@2ac7052a4e02b4a8a0125a106d87465a3b9fd688 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming: we don't need to pin flake8-import-order to this commit anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
Back when i added the environment with the flake8 check there was a bug with one of the rules which was fixed in the repo, but not in the release.
Updated with review-feedback |
.travis.yml
Outdated
include: | ||
- python: 3.6 | ||
env: | ||
- TOX_ENV=py36 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still some indentation that has snuck in here 😛 (basically just remove the spaces that show up in the diff on lines 12-15)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah the good old 'let me just commit this real quick before bed' strikes again...
7c2c5e8
to
d6ffdcd
Compare
* improve wording on testing with fox (contribute.rst) * fix whitespace (.travis.xml)
d6ffdcd
to
490e364
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
See commits.