Skip to content

Linting and starting to use Tox #1450

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

Merged
merged 7 commits into from
Jul 22, 2015
Merged

Linting and starting to use Tox #1450

merged 7 commits into from
Jul 22, 2015

Conversation

agjohnson
Copy link
Contributor

This is a example of some improved linting and testing configuration, thoughts are welcome.

This is an example of linting using Prospector, which is a wrapper around a number of tools -- including flake8 and pylint. It includes a configuration that will set a low bar to getting back to linting code, and is configurable by strictness profile -- which is something we can increase when we start to clean things up more.

Output of a run here:
https://travis-ci.org/rtfd/readthedocs.org/builds/71619840

This also starts to use tox, as suggested by @destroyerofbuilds, which seems like an easy way to at least combine our testing, linting, and doc creation test commands into one tool, instead of one-off shell scripts. It makes for more reproducible tests as well, handling virtualenv creation.

This also fixes a bug on dict ordering in a test case.

@agjohnson agjohnson added Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review labels Jul 19, 2015
@agjohnson agjohnson added this to the Cleanup milestone Jul 19, 2015
setenv =
PYTHONPATH={toxinidir}/readthedocs:{toxinidir}
DJANGO_SETTINGS_MODULE=settings.test
deps = -r{toxinidir}/requirements/pip.txt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, based on depending on requitements.txt that all commands are already executed with {toxinidir} as the working directory, making the inclusion of it in your paths redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicit is better than implicit here.

@ericholscher
Copy link
Member

👍 on this -- we should fix all the errors before merging though, perhaps in another PR on top?

@agjohnson
Copy link
Contributor Author

I'll rebase this up and add some docs. I'll leave the runtests.sh for now, but the docs will reflect running tox locally. I'll open up some tickets against this as well, so some of these issues can be followed up on.

@agjohnson agjohnson force-pushed the tox-test-and-lint branch from e239f7a to 1349be5 Compare July 22, 2015 02:41
@agjohnson agjohnson added PR: ready for review and removed Needed: design decision A core team decision is required PR: work in progress Pull request is not ready for full review labels Jul 22, 2015
@agjohnson
Copy link
Contributor Author

Docs added, this should be ready to go if it looks good.

@ericholscher I added some follow up tickets to address linting and enabling them with travis. Safe to run test suite this way for now and follow up work on other PRs without blocking getting this merged.

@ericholscher
Copy link
Member

Ship it!

ericholscher added a commit that referenced this pull request Jul 22, 2015
Linting and starting to use Tox
@ericholscher ericholscher merged commit c6978f4 into master Jul 22, 2015
@agjohnson agjohnson deleted the tox-test-and-lint branch July 22, 2015 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants