-
Notifications
You must be signed in to change notification settings - Fork 11
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, drop Python 3.3 #3
Conversation
- Run doctests on all versions of Python in tox and travis - Enable coveralls and reach 100% coverage. - Fix broken badge. - Whitespace cleanup in setup.py - Add python_requires
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.
lgtm
Thank you! Can we please get a PyPI release? |
.. image:: https://coveralls.io/repos/github/zopefoundation/zope.hookable/badge.svg?branch=master | ||
:target: https://coveralls.io/github/zopefoundation/zope.hookable?branch=master | ||
|
||
|
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.
ISTM that if one is going to futz around with re-indenting the directive options, one would make them all uniform.
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.
They're visually aligned in the file (at least for me).
What happened was that I copied the first two badges directly out of the ZODB readme, which used a different alignment than the fields here. When I corrected that, they accidentally ended up with a tab character instead of spaces 😢
}, | ||
python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*', |
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.
DRY? We don't need no steenkin' DRY!
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 that wouldn't be inferred from the Trove classifiers is beyond me).
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.
Using the trove classifiers was discussed on distutils-sig earlier this year. Not much came of it, probably because of lack of time.
commands = | ||
coverage run setup.py -q test -q | ||
coverage run -a -m sphinx -b doctest -d {envdir}/.cache/doctrees docs {envdir}/.cache/doctest | ||
coverage report |
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.
How do we signal failure if coverage drops below 100%
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.
tox doesn't. coveralls OTOH, is configured to fail the PR.
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.
@jamadden coverage report
has an option --fail-under=MIN
: "Exit with a status of 2 if the total coverage is less than MIN." which might help to fail on TravisCI, too.
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.
TravisCI doesn't run 'coverage report' because in my opinion coveralls does a better job reporting and failing the build. It's nice to be able to see that one red X for coveralls rather than having to go look at the output of each individual Travis build to see why it failed. (In other words, its good not to conflate test failures with coverage failures.)
I suppose we could add that to tox.ini going forward. It doesn't make much of a difference to me; I tend to look at the coverage report (coverage html
) before pushing anything substantial---I have to do that anyway if coverage drops---but if I forget and coverage drops, coveralls catches it 😄
The primary downside I see is those cases where complete coverage requires several combined tox environment runs (different python versions). I know we have at least two projects that handle that case, but when I tried one of them it didn't work very well for me, at least not using detox
(which is mainly how I run tox) (something was messed up in the report, I don't exactly remember what).
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.
+1
Is there a way to tell detox
to run a specific environment as the last one. This seems to be necessary for a separate coverage
environment.
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.
No, there doesn't seem to be a trivial way to do a combined 'coverage' or 'status' environment under detox while leaving it listed in the default envlist
. There's a feature request for that at tox-dev/tox#234
|
Thank you. 4.1.0 pushed to PyPI. |
Refs zopefoundation/zopetoolkit#8