- Notifications
You must be signed in to change notification settings - Fork294
Update Travis to use tox, and add Appveyor CI#293
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Update tox.ini to utilise requirements-test.txt, and run pylint.Require lxml 3.6.0 on Windows as it has wheels available for 2.6-3.4.Also enables coverage for PyPy on Travis.
Note |
https://ci.appveyor.com/project/jayvdb17618/html5lib-python/build/1.0.22 is an Appveyor CI build of this commit (23c9013). |
codecov-io commentedJul 27, 2016
Current coverage is 90.46% (diff: 100%)@@ master #293 diff @@========================================== Files 50 50 Lines 6930 6931 +1 Methods 0 0 Messages 0 0 Branches 1336 1338 +2 ========================================== Hits 6270 6270 Misses 500 500- Partials 160 161 +1
|
So the reason why we currently don't run tox on Travis is because I'm reluctant to make tox always run coverage: it has a notable performance hit on test execution. Given as far as I'm aware you have a better idea about tox than me—is there any decent way to avoid this? I guess one option is to use Other comments soon to be inline… |
| lxml ; platform_python_implementation == 'CPython' | ||
| lxml ; platform_python_implementation == 'CPython' and sys_platform != 'win32' | ||
| lxml==3.6.0 ; platform_python_implementation == 'CPython' and sys_platform == 'win32' and python_version <= '3.4' | ||
| lxml==3.6.0 ; platform_python_implementation == 'CPython' and sys_platform == 'win_amd64' and python_version <= '3.4' |
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.
why do we have this pinned to 3.6.0? (answers preferably as a comment!)
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 is not mandatory, but highly recommended to simplify life for Windows users.
My reasoning in the commit msg is:
Require lxml 3.6.0 on Windows as it has wheels available for 2.6-3.4.
3.6.1 doesnt have wheels; if we dont pin it, pip will attempt to install 3.6.1 from source.
If you agree with this reasoning, ill add a comment explaining it.
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.
Yeah, please do add a comment!
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.
This is all unnecessary now with later releases oflxml. yay.#363
I've got an idea on how make coverage optional, used on travis but not required on local dev usage. |
| pylint==1.6.4 ; python_version > '2.6' | ||
| pylint==1.3.1 ; python_version <= '2.6' | ||
| astroid==1.3.5 ; python_version <= '2.6' |
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.
What's this?
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.
This was to get Python 2.6 builds to work , as pylint dropped support for Python 2.6 a while ago, hence this workaround.
Anyway,#330 has landed, so that is unnecessary now.
@jayvdb That idea is…? :) |
@jayvdb Seems like there's some work that needs to be done here. Further, over time some of the changed files themselves have changed and there are non-trivial merge conflicts. Do you still want to land this? If so, do you have time to work on it? |
I'll take a look at it today to see if it is worth the effort ;-) |
@jayvdb Thank you! I appreciate it! |
IMO the most important part was getting AppVeyor doing Windows testing, so that part is moved to#359 . I'll stay engaged in getting that patch merged. Then regarding this patch, lots of parts can be dropped now, but there is still a nugget or two to be extracted, and I'll still keen to get that done. Next week ok? |
@jayvdb The premise of this PR is important. We definitely want to switch to tox for managing test environments. I've got some time this week to work on it, so I think I'll pull the bits I want from this PR and throw it in a new PR. Then I'll ping you to review that to make sure I got the bits you wanted done. Would that be ok? |
If it is important, I'll work on it sooner. I'd like to finish this off, if that is OK. (and yes, I'll probably splitting bits off to new PRs, and then kill this one when its all been rescued.) |
@jayvdb I'm totally game for you doing it. It sounded like you didn't have time and I happen to have some time this week. Earlier is better, but next week is fine. Thank you! |
@jayvdb It's been two weeks... how goes it? Is there anything I can do to help? |
I'm putting it together now. Should be a few hours max |
Most of the goodness is now in#364 |
Given the last comment, I'm going to close this out. |
Uh oh!
There was an error while loading.Please reload this page.
Update tox.ini to utilise requirements-test.txt, and run pylint.
Require lxml 3.6.0 on Windows as it has wheels available for 2.6-3.4.
Also enables coverage for PyPy on Travis.